gpu: nova-core: add Turing support

Rename FalconMem::Imem to ImemSec to indicate that it references Secure Instruction Memory. This change has no functional impact.

On Falcon cores, pages in instruction memory can be tagged as Secure or Non-Secure. For GA102 and later, only Secure is used, which is why FalconMem::Imem seems appropriate. However, Turing firmware images can only contain non-secure sections, and so FalconMem needs to support that. By renaming Imem to ImemSec now, future patches for Turing support will be simpler.

Nouveau uses the term "IMEM" to refer both to the Instruction Memory block on Falcon cores as well as to the images of secure firmware uploaded to part of IMEM. OpenRM uses the terms "ImemSec" and "ImemNs" instead, and uses "IMEM" just to refer to the physical memory device.

Renaming these terms allows us to align with OpenRM, avoid confusion between IMEM and ImemSec, and makes future patches simpler.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/falcon.rs          | 14 +++++++-------
 drivers/gpu/nova-core/firmware/booter.rs | 12 ++++++------
 drivers/gpu/nova-core/firmware/fwsec.rs  |  2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

drivers/gpu/nova-core/falcon.rs +7 -7

index 05b124acbfc1..0e0935dbb927 100644
@@ -237,8 +237,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
 /// Different types of memory present in a falcon core.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub(crate) enum FalconMem {
-    /// Instruction Memory.
-    Imem,
+    /// Secure Instruction Memory.
+    ImemSec,
     /// Data Memory.
     Dmem,
 }

@@ -345,8 +345,8 @@ pub(crate) struct FalconBromParams {
 
 /// Trait for providing load parameters of falcon firmwares.
 pub(crate) trait FalconLoadParams {
-    /// Returns the load parameters for `IMEM`.
-    fn imem_load_params(&self) -> FalconLoadTarget;
+    /// Returns the load parameters for Secure `IMEM`.
+    fn imem_sec_load_params(&self) -> FalconLoadTarget;
 
     /// Returns the load parameters for `DMEM`.
     fn dmem_load_params(&self) -> FalconLoadTarget;

@@ -451,7 +451,7 @@ 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::Imem => (load_offsets.src_start, fw.dma_handle()),
+            FalconMem::ImemSec => (load_offsets.src_start, fw.dma_handle()),
             FalconMem::Dmem => (
                 0,
                 fw.dma_handle_with_offset(load_offsets.src_start.into_safe_cast())?,

@@ -502,7 +502,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::Imem)
+            .set_imem(target_mem == FalconMem::ImemSec)
             .set_sec(if sec { 1 } else { 0 });
 
         for pos in (0..num_transfers).map(|i| i * DMA_LEN) {

@@ -538,7 +538,7 @@ pub(crate) fn dma_load<F: FalconFirmware<Target = E>>(&self, bar: &Bar0, fw: &F)
                 .set_mem_type(FalconFbifMemType::Physical)
         });
 
-        self.dma_wr(bar, fw, FalconMem::Imem, fw.imem_load_params(), true)?;
+        self.dma_wr(bar, fw, FalconMem::ImemSec, fw.imem_sec_load_params(), true)?;
         self.dma_wr(bar, fw, FalconMem::Dmem, fw.dmem_load_params(), true)?;
 
         self.hal.program_brom(self, bar, &fw.brom_params())?;

drivers/gpu/nova-core/firmware/booter.rs +6 -6

index f107f753214a..096cd01dbc9d 100644
@@ -251,8 +251,8 @@ impl<'a> FirmwareSignature<BooterFirmware> for BooterSignature<'a> {}
 
 /// The `Booter` loader firmware, responsible for loading the GSP.
 pub(crate) struct BooterFirmware {
-    // Load parameters for `IMEM` falcon memory.
-    imem_load_target: FalconLoadTarget,
+    // Load parameters for Secure `IMEM` falcon memory.
+    imem_sec_load_target: FalconLoadTarget,
     // Load parameters for `DMEM` falcon memory.
     dmem_load_target: FalconLoadTarget,
     // BROM falcon parameters.

@@ -354,7 +354,7 @@ pub(crate) fn new(
         };
 
         Ok(Self {
-            imem_load_target: FalconLoadTarget {
+            imem_sec_load_target: FalconLoadTarget {
                 src_start: app0.offset,
                 dst_start: 0,
                 len: app0.len,

@@ -371,8 +371,8 @@ pub(crate) fn new(
 }
 
 impl FalconLoadParams for BooterFirmware {
-    fn imem_load_params(&self) -> FalconLoadTarget {
-        self.imem_load_target.clone()
+    fn imem_sec_load_params(&self) -> FalconLoadTarget {
+        self.imem_sec_load_target.clone()
     }
 
     fn dmem_load_params(&self) -> FalconLoadTarget {

@@ -384,7 +384,7 @@ fn brom_params(&self) -> FalconBromParams {
     }
 
     fn boot_addr(&self) -> u32 {
-        self.imem_load_target.src_start
+        self.imem_sec_load_target.src_start
     }
 }
 

drivers/gpu/nova-core/firmware/fwsec.rs +1 -1

index b28e34d279f4..6a2f5a0d4b15 100644
@@ -224,7 +224,7 @@ pub(crate) struct FwsecFirmware {
 }
 
 impl FalconLoadParams for FwsecFirmware {
-    fn imem_load_params(&self) -> FalconLoadTarget {
+    fn imem_sec_load_params(&self) -> FalconLoadTarget {
         FalconLoadTarget {
             src_start: 0,
             dst_start: self.desc.imem_phys_base,

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
 drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++--
 drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++
 drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++
 3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +16 -2

index 0e0935dbb927..ece8b92a627e 100644
@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
 pub(crate) enum FalconMem {
     /// Secure Instruction Memory.
     ImemSec,
+    /// Non-Secure Instruction Memory.
+    ImemNs,
     /// Data Memory.
     Dmem,
 }

@@ -348,6 +350,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;
 

@@ -451,7 +457,7 @@ 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::ImemSec => (load_offsets.src_start, fw.dma_handle()),
+            FalconMem::ImemSec | FalconMem::ImemNs => (load_offsets.src_start, fw.dma_handle()),
             FalconMem::Dmem => (
                 0,
                 fw.dma_handle_with_offset(load_offsets.src_start.into_safe_cast())?,

@@ -502,7 +508,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::ImemSec)
+            .set_imem(target_mem != FalconMem::Dmem)
             .set_sec(if sec { 1 } else { 0 });
 
         for pos in (0..num_transfers).map(|i| i * DMA_LEN) {

@@ -541,6 +547,14 @@ pub(crate) fn dma_load<F: FalconFirmware<Target = E>>(&self, bar: &Bar0, fw: &F)
         self.dma_wr(bar, fw, FalconMem::ImemSec, fw.imem_sec_load_params(), true)?;
         self.dma_wr(bar, fw, FalconMem::Dmem, fw.dmem_load_params(), true)?;
 
+        if let Some(nmem) = fw.imem_ns_load_params() {
+            // This code should never actual get executed, because the ImemNs
+            // section only exists on firmware used by Turing and GA100, and
+            // those platforms do not use DMA.  But we include this code for
+            // consistency.
+            self.dma_wr(bar, fw, FalconMem::ImemNs, nmem, false)?;
+        }
+
         self.hal.program_brom(self, bar, &fw.brom_params())?;
 
         // Set `BootVec` to start of non-secure code.

drivers/gpu/nova-core/firmware/booter.rs +9 -0

index 096cd01dbc9d..1b98bb47424c 100644
@@ -253,6 +253,9 @@ impl<'a> FirmwareSignature<BooterFirmware> for BooterSignature<'a> {}
 pub(crate) struct BooterFirmware {
     // Load parameters for Secure `IMEM` falcon memory.
     imem_sec_load_target: FalconLoadTarget,
+    // Load parameters for Non-Secure `IMEM` falcon memory,
+    // used only on Turing and GA100
+    imem_ns_load_target: Option<FalconLoadTarget>,
     // Load parameters for `DMEM` falcon memory.
     dmem_load_target: FalconLoadTarget,
     // BROM falcon parameters.

@@ -359,6 +362,8 @@ pub(crate) fn new(
                 dst_start: 0,
                 len: app0.len,
             },
+            // 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,

@@ -375,6 +380,10 @@ fn imem_sec_load_params(&self) -> FalconLoadTarget {
         self.imem_sec_load_target.clone()
     }
 
+    fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
+        self.imem_ns_load_target.clone()
+    }
+
     fn dmem_load_params(&self) -> FalconLoadTarget {
         self.dmem_load_target.clone()
     }

drivers/gpu/nova-core/firmware/fwsec.rs +5 -0

index 6a2f5a0d4b15..e4009faba6c5 100644
@@ -232,6 +232,11 @@ fn imem_sec_load_params(&self) -> FalconLoadTarget {
         }
     }
 
+    fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
+        // Only used on Turing and GA100, so return None for now
+        None
+    }
+
     fn dmem_load_params(&self) -> FalconLoadTarget {
         FalconLoadTarget {
             src_start: self.desc.imem_load_size,

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
 drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++--
 drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++
 drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++
 3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +0 -0

index 0e0935dbb927..ece8b92a627e 100644

Quoted 17 lines · Timur Tabi

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
  drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++--
  drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++
  drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++
  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +0 -0

index 0e0935dbb927..ece8b92a627e 100644

drivers/gpu/nova-core/falcon.rs fn from(value: PeregrineCoreSelect) -> Self { · line 243

Quoted 5 lines

drivers/gpu/nova-core/falcon.rs +2 -0

@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
  pub(crate) enum FalconMem {
      /// Secure Instruction Memory.
      ImemSec,
+    /// Non-Secure Instruction Memory.
+    ImemNs,

So, seeing how this is taking shape I now think we should just have one Imem variant:

Imem { secure: bool },

This makes matching easier for the common case of "we want to do something in case of Imem, regardless of the secure flag". Something like

     FalconMem::ImemSec | FalconMem::ImemNs => {

becomes:

     FalconMem::Imem { .. } => {

And if you need to use the flag, you can change e.g.:

     FalconMem::ImemSec | FalconMem::ImemNs => {
         regs::NV_PFALCON_FALCON_IMEMC::default()
             .set_secure(target_mem == FalconMem::ImemSec)

into

     FalconMem::Imem { secure } => {

regs::NV_PFALCON_FALCON_IMEMC::default()
.set_secure(secure)

Which is simpler and easier to read.

This also removes the need to rename Imem into ImemSec, so the first two patches can be merged into one.

Quoted 27 lines · Alexandre Courbot

Quoted 17 lines · Timur Tabi

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
  drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++--
  drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++
  drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++
  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +0 -0

index 0e0935dbb927..ece8b92a627e 100644

drivers/gpu/nova-core/falcon.rs fn from(value: PeregrineCoreSelect) -> Self { · line 243

Quoted 5 lines

drivers/gpu/nova-core/falcon.rs +2 -0

@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
  pub(crate) enum FalconMem {
      /// Secure Instruction Memory.
      ImemSec,
+    /// Non-Secure Instruction Memory.
+    ImemNs,

So, seeing how this is taking shape I now think we should just have one Imem variant:

Quoted 1 line

Imem { secure: bool },

ohhh, boolean args are usually not a good idea, because they make the callsite non-self-documenting.

That's also true even in magical languages such as Rust. :)

Let's prefer enum args over bools, generally, please. So for example (there are other ways to structure things, and this is just the enum aspect of it):

     enum ImemSecurity {
         Secure,
         NonSecure,
     }

    Imem { security: ImemSecurity },

Quoted 18 lines

This makes matching easier for the common case of "we want to do something in case of Imem, regardless of the secure flag". Something like

     FalconMem::ImemSec | FalconMem::ImemNs => {

becomes:

     FalconMem::Imem { .. } => {

And if you need to use the flag, you can change e.g.:

     FalconMem::ImemSec | FalconMem::ImemNs => {
         regs::NV_PFALCON_FALCON_IMEMC::default()
             .set_secure(target_mem == FalconMem::ImemSec)

Quoted 3 lines

into

     FalconMem::Imem { secure } => {

See, this is hard and misleading to read. It reads like "secure Imem", until you think at it a bit. Devastating! :)

Quoted 28 lines · John Hubbard

Quoted 27 lines · Alexandre Courbot

Quoted 17 lines · Timur Tabi

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
  drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++-- drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++ drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++

  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +0 -0

index 0e0935dbb927..ece8b92a627e 100644

drivers/gpu/nova-core/falcon.rs fn from(value: PeregrineCoreSelect) -> Self { · line 243

Quoted 5 lines

drivers/gpu/nova-core/falcon.rs +2 -0

@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
   pub(crate) enum FalconMem {
       /// Secure Instruction Memory.
       ImemSec,
+    /// Non-Secure Instruction Memory.
+    ImemNs,

So, seeing how this is taking shape I now think we should just have one Imem variant:

Quoted 6 lines

Quoted 1 line

     Imem { secure: bool },

ohhh, boolean args are usually not a good idea, because they make the callsite non-self-documenting.

That's also true even in magical languages such as Rust. :)

I fully agree; that's why I made the field named so its name needs to be specified every time. :) Maybe is_secure would have been better though.

Quoted 11 lines

Let's prefer enum args over bools, generally, please. So for example (there are other ways to structure things, and this is just the enum aspect of it):

     enum ImemSecurity {
         Secure,
         NonSecure,
     }

    Imem { security: ImemSecurity },

That would change

    FalconMem::Imem { secure: true }

into

    FalconMem::Imem {security: ImemSecurity::Secure }

If we want to use an enum I think we can remove the name:

     Imem(ImemSecurity),

So we can specify Imem as

    FalconMem::Imem(ImemSecurity::Secure)

which is as explicit, and a bit shorter.

Quoted 19 lines

Quoted 19 lines

This makes matching easier for the common case of "we want to do something in case of Imem, regardless of the secure flag". Something like

     FalconMem::ImemSec | FalconMem::ImemNs => {

becomes:

     FalconMem::Imem { .. } => {

And if you need to use the flag, you can change e.g.:

     FalconMem::ImemSec | FalconMem::ImemNs => { regs::NV_PFALCON_FALCON_IMEMC::default() .set_secure(target_mem == FalconMem::ImemSec)

Quoted 6 lines

Quoted 3 lines

into

     FalconMem::Imem { secure } => {

See, this is hard and misleading to read. It reads like "secure Imem", until you think at it a bit. Devastating! :)

Renaming into is_secure would alleviate that, but the ImemSecurity enum is arguably as good, so I'm fine with it as well.

And an enum can also be used as a type to method arguments, which carries more semantics than is_secure: bool. So agreed, this is better.

Quoted 62 lines · Alexandre Courbot

Quoted 28 lines · John Hubbard

Quoted 27 lines · Alexandre Courbot

Quoted 17 lines · Timur Tabi

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
  drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++-- drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++ drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++

  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +0 -0

index 0e0935dbb927..ece8b92a627e 100644

drivers/gpu/nova-core/falcon.rs fn from(value: PeregrineCoreSelect) -> Self { · line 243

Quoted 5 lines

drivers/gpu/nova-core/falcon.rs +2 -0

@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
   pub(crate) enum FalconMem {
       /// Secure Instruction Memory.
       ImemSec,
+    /// Non-Secure Instruction Memory.
+    ImemNs,

So, seeing how this is taking shape I now think we should just have one Imem variant:

Quoted 6 lines

Quoted 1 line

     Imem { secure: bool },

ohhh, boolean args are usually not a good idea, because they make the callsite non-self-documenting.

That's also true even in magical languages such as Rust. :)

I fully agree; that's why I made the field named so its name needs to be specified every time. :) Maybe is_secure would have been better though.

Quoted 11 lines

Let's prefer enum args over bools, generally, please. So for example (there are other ways to structure things, and this is just the enum aspect of it):

     enum ImemSecurity {
         Secure,
         NonSecure,
     }

    Imem { security: ImemSecurity },

That would change

    FalconMem::Imem { secure: true }

into

    FalconMem::Imem {security: ImemSecurity::Secure }

If we want to use an enum I think we can remove the name:

Quoted 7 lines

     Imem(ImemSecurity),

So we can specify Imem as

    FalconMem::Imem(ImemSecurity::Secure)

which is as explicit, and a bit shorter.

A struct could be another option? You have 2 entities here, the location of the memory (instruction memory or data memory) and the secure aspect.

struct FalconMem {
   type: FalconMemType,  // enum which can be instruction or data
   security: FalconMemSecurity, // enum can be secure or insecure. }

That documents everything. But it is just an option I am putting out to consider if it helps.

Quoted 8 lines · Joel Fernandes

A struct could be another option? You have 2 entities here, the location of the memory (instruction memory or data memory) and the secure aspect.

struct FalconMem {
   type: FalconMemType,  // enum which can be instruction or data
   security: FalconMemSecurity, // enum can be secure or insecure. }

Quoted 2 lines

That documents everything. But it is just an option I am putting out to consider if it helps.

Sure, but the security only applies to Imem, not Dmem. I didn't want to come up with a design that allowed for "Secure Dmem", like your FalconMem struct does. That's why I think it makes more sense to have just two Imem types.

Have we come full circle, back to the original patch, but with slightly longer names (please)? (Spell out Secure and NonSecure, rather than typing Sec and Ns.)

Quoted 64 lines · Alexandre Courbot

Quoted 28 lines · John Hubbard

Quoted 27 lines · Alexandre Courbot

Quoted 17 lines · Timur Tabi

The GSP booter firmware in Turing and GA100 includes a third memory section called ImemNs, 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 <[email protected]>

---
  drivers/gpu/nova-core/falcon.rs          | 18 ++++++++++++++++-- drivers/gpu/nova-core/firmware/booter.rs |  9 +++++++++ drivers/gpu/nova-core/firmware/fwsec.rs  |  5 +++++

  3 files changed, 30 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/falcon.rs +0 -0

index 0e0935dbb927..ece8b92a627e 100644

drivers/gpu/nova-core/falcon.rs fn from(value: PeregrineCoreSelect) -> Self { · line 243

Quoted 5 lines

drivers/gpu/nova-core/falcon.rs +2 -0

@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
   pub(crate) enum FalconMem {
       /// Secure Instruction Memory.
       ImemSec,
+    /// Non-Secure Instruction Memory.
+    ImemNs,

So, seeing how this is taking shape I now think we should just have one Imem variant:

Quoted 6 lines

Quoted 1 line

     Imem { secure: bool },

ohhh, boolean args are usually not a good idea, because they make the callsite non-self-documenting.

That's also true even in magical languages such as Rust. :)

I fully agree; that's why I made the field named so its name needs to be specified every time. :) Maybe is_secure would have been better though.

Quoted 11 lines

Let's prefer enum args over bools, generally, please. So for example (there are other ways to structure things, and this is just the enum aspect of it):

     enum ImemSecurity {
         Secure,
         NonSecure,
     }

    Imem { security: ImemSecurity },

That would change

    FalconMem::Imem { secure: true }

into

    FalconMem::Imem {security: ImemSecurity::Secure }

If we want to use an enum I think we can remove the name:

     Imem(ImemSecurity),

Quoted 5 lines

So we can specify Imem as

    FalconMem::Imem(ImemSecurity::Secure)

which is as explicit, and a bit shorter.

I fail to see how any of this is an improvement.

It lets you capture both Imem variants with a single match arm instead of having to remember to use |, a pattern that is common in this series.

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 <[email protected]>

---
 drivers/gpu/nova-core/firmware/booter.rs | 40 +++++++++++++++++++-----
 1 file changed, 33 insertions(+), 7 deletions(-)

drivers/gpu/nova-core/firmware/booter.rs +0 -0

index 1b98bb47424c..6ac9593504db 100644

drivers/gpu/nova-core/firmware/booter.rs +3 -4

+
         Ok(Self {
-            imem_sec_load_target: FalconLoadTarget {
-                src_start: app0.offset,
-                dst_start: 0,
-                len: app0.len,
+            imem_sec_load_target: if chipset > Chipset::GA100 {
+                FalconLoadTarget {

drivers/gpu/nova-core/firmware/booter.rs +9 -2

+            },
+            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,

drivers/gpu/nova-core/firmware/booter.rs +2 -0

+            self.imem_sec_load_target.src_start
+        }
     }
 }
 

Signature · 1 line

2.51.2

Quoted 5 lines

+        // 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.

Some more doc comments and documentation explaining the header structure would be great.

Quoted 10 lines

+
         Ok(Self {
-            imem_sec_load_target: FalconLoadTarget { -                src_start: app0.offset,
-                dst_start: 0,
-                len: app0.len,
+            imem_sec_load_target: if chipset > Chipset::GA100 { +                FalconLoadTarget {
+                    src_start: app0.offset,
+                    dst_start: 0,

Quoted 8 lines

+                    len: app0.len,
+                }
+            } else {
+                FalconLoadTarget {
+                    src_start: load_hdr.os_code_size, +                    dst_start: app0.offset, +                    len: app0.len,
+                }

Can write more succinctly:

  imem_sec_load_target: FalconLoadTarget {
      src_start: match chipset > Chipset::GA100  { true => app0.offset,
          false => load_hdr.os_code_size, },
      dst_start: match chipset > Chipset::GA100 { true => 0,
          false => app0.offset,

      len: app0.len,
  },

Quoted 21 lines

+            },
+            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,

(quoted diff) +0 -0

@@ -393,7 +413,13 @@ fn brom_params(&self) -> FalconBromParams {
      }

     fn boot_addr(&self) -> u32 {
-        self.imem_sec_load_target.src_start

Quoted 5 lines

+        if let Some(ns_target) = &self.imem_ns_load_target { +            // Turing and GA100 - use non-secure load target +            ns_target.dst_start
+        } else {
+            // GA102+ (Ampere) - use secure load target

s/Ampere/Ampere and later/ ? Also missing period at end of comment, here and elsewhere.

Quoted 21 lines

Quoted 10 lines

+
         Ok(Self {
-            imem_sec_load_target: FalconLoadTarget { -                src_start: app0.offset,
-                dst_start: 0,
-                len: app0.len,
+            imem_sec_load_target: if chipset > Chipset::GA100 { +                FalconLoadTarget {
+                    src_start: app0.offset,
+                    dst_start: 0,

Quoted 8 lines

+                    len: app0.len,
+                }
+            } else {
+                FalconLoadTarget {
+                    src_start: load_hdr.os_code_size, +                    dst_start: app0.offset, +                    len: app0.len,
+                }

Can write more succinctly:

Quoted 11 lines

  imem_sec_load_target: FalconLoadTarget {
      src_start: match chipset > Chipset::GA100  { true => app0.offset,
          false => load_hdr.os_code_size, },
      dst_start: match chipset > Chipset::GA100 { true => 0,
          false => app0.offset,

      len: app0.len,
  },

Do we really want to use "match" instead of "if", just because we don't need "else"?

I don't care about the if/else as much as I care about the opportunity to just specify FalconLoadTarget once instead twice. I think the match here is cleaner for this snippet, but I am Ok with the if/else as well.

Something like:
   imem_sec_load_target: FalconLoadTarget {
       src_start: if chipset > Chipset::GA100  {
          app0.offset
      } else {
           load_hdr.os_code_size
      },

That would be one more line of code, but pretty much the same.

Quoted 12 lines · Joel Fernandes

I don't care about the if/else as much as I care about the opportunity to just specify FalconLoadTarget once instead twice. I think the match here is cleaner for this snippet, but I am Ok with the if/else as well.

Something like:
   imem_sec_load_target: FalconLoadTarget {
       src_start: if chipset > Chipset::GA100  {
          app0.offset
      } else {
           load_hdr.os_code_size
      },

Quoted 1 line

That would be one more line of code, but pretty much the same.

Interesting. I would have thought that duplicating the if-statement is the higher offense.

Quoted 15 lines · Timur Tabi

Quoted 13 lines · Joel Fernandes

I don't care about the if/else as much as I care about the opportunity to just specify FalconLoadTarget once instead twice. I think the match here is cleaner for this snippet, but I am Ok with the if/else as well.

Something like:
   imem_sec_load_target: FalconLoadTarget {
       src_start: if chipset > Chipset::GA100  {
          app0.offset
      } else {
           load_hdr.os_code_size
      },

That would be one more line of code, but pretty much the same.

Quoted 1 line

Interesting. I would have thought that duplicating the if-statement is the higher offense.

I think the use of traits here generally makes this all a little better overall though, don't you think?

Traits are easier to read and internalize quickly, and easy to maintain too. They are yet another way to make implicit rules explicit and self-documenting.

On Tue, Nov 18, 2025 at 12:52:47AM +0000, Timur Tabi wrote:
[...]

Quoted 33 lines

Quoted 21 lines

Quoted 10 lines

+
         Ok(Self {
-            imem_sec_load_target: FalconLoadTarget { -                src_start: app0.offset,
-                dst_start: 0,
-                len: app0.len,
+            imem_sec_load_target: if chipset > Chipset::GA100 { +                FalconLoadTarget {
+                    src_start: app0.offset,
+                    dst_start: 0,

Quoted 8 lines

+                    len: app0.len,
+                }
+            } else {
+                FalconLoadTarget {
+                    src_start: load_hdr.os_code_size, +                    dst_start: app0.offset, +                    len: app0.len,
+                }

Can write more succinctly:

Quoted 11 lines

  imem_sec_load_target: FalconLoadTarget {
      src_start: match chipset > Chipset::GA100  { true => app0.offset,
          false => load_hdr.os_code_size, },
      dst_start: match chipset > Chipset::GA100 { true => 0,
          false => app0.offset,

      len: app0.len,
  },

Quoted 1 line

Do we really want to use "match" instead of "if", just because we don't need "else"?

I don't care about the if/else as much as I care about the opportunity to just specify FalconLoadTarget once instead twice. I think the match here is cleaner for this snippet, but I am Ok with the if/else as well.

Something like:

   imem_sec_load_target: FalconLoadTarget {
       src_start: if chipset > Chipset::GA100  {
          app0.offset
      } else {
           load_hdr.os_code_size
      },

That would be one more line of code, but pretty much the same.

Quoted 42 lines · Joel Fernandes

On Tue, Nov 18, 2025 at 12:52:47AM +0000, Timur Tabi wrote:
[...]

Quoted 33 lines

Quoted 21 lines

Quoted 10 lines

+
         Ok(Self {
-            imem_sec_load_target: FalconLoadTarget { -                src_start: app0.offset,
-                dst_start: 0,
-                len: app0.len,
+            imem_sec_load_target: if chipset > Chipset::GA100 { +                FalconLoadTarget {
+                    src_start: app0.offset,
+                    dst_start: 0,

Quoted 8 lines

+                    len: app0.len,
+                }
+            } else {
+                FalconLoadTarget {
+                    src_start: load_hdr.os_code_size, +                    dst_start: app0.offset, +                    len: app0.len,
+                }

Can write more succinctly:

Quoted 11 lines

  imem_sec_load_target: FalconLoadTarget {
      src_start: match chipset > Chipset::GA100  { true => app0.offset,
          false => load_hdr.os_code_size, },
      dst_start: match chipset > Chipset::GA100 { true => 0,
          false => app0.offset,

      len: app0.len,
  },

Quoted 1 line

Do we really want to use "match" instead of "if", just because we don't need "else"?

I don't care about the if/else as much as I care about the opportunity to just specify FalconLoadTarget once instead twice. I think the match here is cleaner for this snippet, but I am Ok with the if/else as well.

Something like:

Quoted 9 lines

   imem_sec_load_target: FalconLoadTarget {
       src_start: if chipset > Chipset::GA100  {
          app0.offset
      } else {
           load_hdr.os_code_size
      },

That would be one more line of code, but pretty much the same.

You know, this latest snippet looks a little better. The pattern of

match a > b {
true => foo,
false => bar,
}

is actually not as nice as an if-else, because, well, anytime you explicitly compare against true and false, you are likely doing something that the language (any language) has a construct for.

And in this case, it appears to be "if/else". :)

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 <[email protected]>

---
 drivers/gpu/nova-core/firmware/booter.rs | 40 +++++++++++++++++++-----
 1 file changed, 33 insertions(+), 7 deletions(-)

drivers/gpu/nova-core/firmware/booter.rs +16 -4

index 1b98bb47424c..6ac9593504db 100644
@@ -356,14 +356,34 @@ 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.
+
         Ok(Self {
-            imem_sec_load_target: FalconLoadTarget {
-                src_start: app0.offset,
-                dst_start: 0,
-                len: app0.len,
+            imem_sec_load_target: if chipset > Chipset::GA100 {
+                FalconLoadTarget {
+                    src_start: app0.offset,
+                    dst_start: 0,
+                    len: app0.len,
+                }
+            } else {
+                FalconLoadTarget {
+                    src_start: load_hdr.os_code_size,
+                    dst_start: app0.offset,
+                    len: app0.len,

I'd prefer if we could reason in terms of functionality instead of specific chipset versions.

IIUC the relevant factor is that Turing/GA100 have some non-secure bootloader code as the entry point of booter, which GA102+ doesn't feature as it is capable of starting in secure mode directly (please correct me as my understanding is probably incomplete if not outright wrong).

What is the HW or SW fact that requires this on Turing?

Is it linked
to the fact we need to use PIO for it? What I would like to achieve is removing or at least reducing these chipset checks into one single point, which in the worst case could be a method of Chipset telling us which loading method to use. But if we can find a distinguishing factor in the parsed by this method, that would be even better.

Quoted 2 lines · Alexandre Courbot

I'd prefer if we could reason in terms of functionality instead of specific chipset versions.

If you can figure it out, I'd be happy to change the code. I wasn't being lazy when I made this comment:

// 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.

Quoted 6 lines

IIUC the relevant factor is that Turing/GA100 have some non-secure bootloader code as the entry point of booter, which GA102+ doesn't feature as it is capable of starting in secure mode directly (please correct me as my understanding is probably incomplete if not outright wrong).

That sounds about right. There are secure and non-secure sections in the firmware image.

Quoted 1 line

What is the HW or SW fact that requires this on Turing?

I don't know how to answer that question. That's just how it's done on Turing/GA100. I would need to start an internal Slack thread to get a better answer, and I don't really see what it would gain us.

Quoted 7 lines

Is it linked
to the fact we need to use PIO for it? What I would like to achieve is removing or at least reducing these chipset checks into one single point, which in the worst case could be a method of Chipset telling us which loading method to use. But if we can find a distinguishing factor in the parsed by this method, that would be even better.

Both OpenRM and Nouveau use the chipset to gate on how to parse the headers.

If it comes down to "This is how things are pre and post GA102" (and the evidence I have seen to far suggests that unfortunately), then so be it
- we at the very least encode this as a method of Chipset to avoid hardcoding chipset versions in several places.

Quoted 4 lines · Alexandre Courbot

If it comes down to "This is how things are pre and post GA102" (and the evidence I have seen to far suggests that unfortunately), then so be it
- we at the very least encode this as a method of Chipset to avoid hardcoding chipset versions in several places.

I could add this method to Chipset:

    pub(crate) fn supports_dma(&self) -> bool {
	*self > Self::GA100
    }

But shouldn't this be added to the HAL instead?

There are a few things that are arbitrarily conditional:

1) whether to use PIO or DMA to transfer firmware images to Falcons

2) whether a Generic Bootloader is needed for load FWSEC

3) whether a Non-Secure IMEM section exists

4) what the name of the signature section in the GSP-RM ELF image is

If I'm reading it right, booter.rs currently has no concept of a HAL, which is why I gated on Chipset. I agree that doing stuff like

                FalconLoadTarget {
                    src_start: app0.offset,
                    dst_start: if chipset > Chipset::GA100 { 0 } else { app0.offset },
                    len: app0.len,
                },

is not ideal. To be honest, it's hard to wrap my head around all of these offsets. I'm convinced that most of the confusion is the result of RM developers trying to shoehorn complex firmware images into legacy file formats. Just looking at the above:

On Turing, the secure IMEM image is copied from app0.offset in the file to -> app0.offset (256) in IMEM.
On Ampere, the secure IMEM image is copied from app0.offset in the file to -> 0 in IMEM.

Why? Apparently, because on Ampere we copy the Secure image to offset 0 and ignore the Non-Secure image. On Turing, we copy the Non-Secure image to offset 0 and copy the Secure image to offset 256. It would have been much easier to copy the Secure image to 256 on Ampere also, but that's just not what happens.

For some reason, os_code_offset and os_code_size contain the same values on both Turing and Ampere, but those values are used only on Turing to hold the Non-Secure section. So the firmware image headers say that there's a non-secure IMEM section on Ampere, but it's a lie.

Quoted 2 lines · Alexandre Courbot

I'd prefer if we could reason in terms of functionality instead of specific chipset versions.

If you can figure it out, I'd be happy to change the code. I wasn't being lazy when I made this comment:

   // 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.

Quoted 5 lines

IIUC the relevant factor is that Turing/GA100 have some non-secure bootloader code as the entry point of booter, which GA102+ doesn't feature as it is capable of starting in secure mode directly (please correct me as my understanding is probably incomplete if not outright wrong).

That sounds about right. There are secure and non-secure sections in the firmware image.

Quoted 1 line

What is the HW or SW fact that requires this on Turing?

I don't know how to answer that question. That's just how it's done on Turing/GA100. I would need to start an internal Slack thread to get a better answer, and I don't really see what it would gain us.

Quoted 7 lines

Is it linked
to the fact we need to use PIO for it? What I would like to achieve is removing or at least reducing these chipset checks into one single point, which in the worst case could be a method of Chipset telling us which loading method to use. But if we can find a distinguishing factor in the parsed by this method, that would be even better.

Both OpenRM and Nouveau use the chipset to gate on how to parse the headers.

Quoted 23 lines · Timur Tabi

Quoted 2 lines · Alexandre Courbot

I'd prefer if we could reason in terms of functionality instead of specific chipset versions.

If you can figure it out, I'd be happy to change the code.  I wasn't being lazy when I made this
comment:

   // 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.

Quoted 5 lines

IIUC the relevant factor is that Turing/GA100 have some non-secure bootloader code as the entry point of booter, which GA102+ doesn't feature as it is capable of starting in secure mode directly (please correct me as my understanding is probably incomplete if not outright wrong).

That sounds about right.  There are secure and non-secure sections in the firmware image.

Quoted 7 lines

Quoted 1 line

What is the HW or SW fact that requires this on Turing?

I don't know how to answer that question.  That's just how it's done on Turing/GA100.  I would
need to start an internal Slack thread to get a better answer, and I don't really see what it
would gain us.

Here, would it be reasonable to just create a tiny HAL-like construct (a trait, I guess) that has the function to call, and it decides which behavior to use based on the chipset-selected HAL.

In other words, we know very clearly that we need to call one version of the function for earlier chips, and the other version of the function for later chips, and the dividing line is because that behavior changed at a certain chipset.

I think this HAL-centric, chipset-centric approach is fine for some things, where we really have no reason to care about the internal details.

For other cases, the functionality based partitioning is probably ideal.

         sec2_falcon.reset(bar)?;
-        if chipset > Chipset::GA100 {
+        if booter_loader.flags() == BooterFlags::Secure { sec2_falcon.dma_load(bar, &booter_loader)?; } else {
             sec2_falcon.pio_load(bar, &booter_loader, None)?;

Quoted 6 lines · Joel Fernandes

         sec2_falcon.reset(bar)?;
-        if chipset > Chipset::GA100 {
+        if booter_loader.flags() == BooterFlags::Secure { sec2_falcon.dma_load(bar, &booter_loader)?; } else {
             sec2_falcon.pio_load(bar, &booter_loader, None)?;

The problem with this is that it conflates (Non-)Secure IMEM with using PIO/DMA. I haven't looked at GA100 yet, but it's just coincidence that platforms that have Non-Secure IMEM sections also don't have (functioning) DMA hardware.

Quoted 8 lines · Timur Tabi

Quoted 6 lines · Joel Fernandes

         sec2_falcon.reset(bar)?;
-        if chipset > Chipset::GA100 {
+        if booter_loader.flags() == BooterFlags::Secure { sec2_falcon.dma_load(bar, &booter_loader)?; } else {
             sec2_falcon.pio_load(bar, &booter_loader, None)?;

Quoted 4 lines

The problem with this is that it conflates (Non-)Secure IMEM with using PIO/DMA. I haven't looked at GA100 yet, but it's just coincidence that platforms that have Non-Secure IMEM sections also don't have (functioning) DMA hardware.

Hopper/Blackwell+ use PIO too. So the conflating could be a real concern.

Quoted 2 lines

Hopper/Blackwell+ use PIO too. So the conflating could be a real concern.

Ok, so we really want to have this handled by the falcon HAL as Timur suggested.

Quoted 6 lines · Joel Fernandes

         sec2_falcon.reset(bar)?;
-        if chipset > Chipset::GA100 {
+        if booter_loader.flags() == BooterFlags::Secure { sec2_falcon.dma_load(bar, &booter_loader)?; } else {
             sec2_falcon.pio_load(bar, &booter_loader, None)?;

The problem with this is that it conflates (Non-)Secure IMEM with using PIO/DMA. I haven't looked at GA100 yet, but it's just coincidence that platforms that have Non-Secure IMEM sections also don't have (functioning) DMA hardware.

Turing and GA100 share the same GSP-RM firmware binary, and the signature ELF section is labeled ".fwsignature_tu10x".

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/firmware/gsp.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/firmware/gsp.rs +7 -2

index 0549805282ab..aa5a6433c377 100644
@@ -163,9 +163,14 @@ pub(crate) fn new<'a, 'b>(
         let fw_section = elf::elf64_section(fw.data(), ".fwimage").ok_or(EINVAL)?;
 
         let sigs_section = match chipset.arch() {
-            Architecture::Ampere => ".fwsignature_ga10x",
+            Architecture::Turing | Architecture::Ampere =>
+                if chipset > Chipset::GA100 {
+                    ".fwsignature_ga10x"
+                } else {
+                    // GA100 uses the same firmware as Turing
+                    ".fwsignature_tu10x"
+                },
             Architecture::Ada => ".fwsignature_ad10x",
-            _ => return Err(ENOTSUPP),
         };
         let signatures = elf::elf64_section(fw.data(), sigs_section)
             .ok_or(EINVAL)

Turing and GA100 share the same GSP-RM firmware binary, and the signature ELF section is labeled ".fwsignature_tu10x".

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/firmware/gsp.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/firmware/gsp.rs +0 -0

index 0549805282ab..aa5a6433c377 100644
@@ -163,9 +163,14 @@ pub(crate) fn new<'a, 'b>(
         let fw_section = elf::elf64_section(fw.data(), ".fwimage").ok_or(EINVAL)?;

Following up the point I raised on patch 3, this could be another site where we use a potential Chipset::has_bootloader_thing or any more fittingly named method.

Quoted 15 lines · Timur Tabi

Turing and GA100 share the same GSP-RM firmware binary, and the signature ELF section is labeled ".fwsignature_tu10x".

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/firmware/gsp.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/gpu/nova-core/firmware/gsp.rs +0 -0

index 0549805282ab..aa5a6433c377 100644
@@ -163,9 +163,14 @@ pub(crate) fn new<'a, 'b>(
         let fw_section = elf::elf64_section(fw.data(), ".fwimage").ok_or(EINVAL)?;
 

drivers/gpu/nova-core/firmware/gsp.rs pub(crate) fn new<'a, 'b>( · line 172

Quoted 9 lines

drivers/gpu/nova-core/firmware/gsp.rs +7 -1

         let sigs_section = match chipset.arch() {
-            Architecture::Ampere => ".fwsignature_ga10x",
+            Architecture::Turing | Architecture::Ampere =>
+                if chipset > Chipset::GA100 {
+                    ".fwsignature_ga10x"
+                } else {
+                    // GA100 uses the same firmware as Turing
+                    ".fwsignature_tu10x"
+                },

Following up the point I raised on patch 3, this could be another site where we use a potential Chipset::has_bootloader_thing or any more fittingly named method.

Quoted 1 line · Alexandre Courbot

On Sat Nov 15, 2025 at 8:30 AM JST, Timur Tabi wrote:

Quoted 10 lines

drivers/gpu/nova-core/firmware/gsp.rs pub(crate) fn new<'a, 'b>( · line 172

Quoted 9 lines

          let sigs_section = match chipset.arch() {

drivers/gpu/nova-core/firmware/gsp.rs +7 -1

-            Architecture::Ampere => ".fwsignature_ga10x",
+            Architecture::Turing | Architecture::Ampere =>
+                if chipset > Chipset::GA100 {
+                    ".fwsignature_ga10x"
+                } else {
+                    // GA100 uses the same firmware as Turing
+                    ".fwsignature_tu10x"
+                },

Quoted 3 lines

Following up the point I raised on patch 3, this could be another site where we use a potential Chipset::has_bootloader_thing or any more fittingly named method.

Ah right, that's unrelated. In this case can we just express the exception as follows:

     let sigs_section = match chipset.arch() {
         Architecture::Turing => ".fwsignature_tu10x",
         // GA100 uses the same firmware name as Turing
         Architecture::Ampere if chipset > Chipset::GA100 => ".fwsignature_tu10x",

         Architecture::Ampere => ".fwsignature_ga10x",
         Architecture::Ada => ".fwsignature_ad10x",
     };

It treats GA100 as an exception instead of complicating the match for both Turing and Ampere.

Quoted 2 lines · Alexandre Courbot

Quoted 1 line · Alexandre Courbot

On Sat Nov 15, 2025 at 8:30 AM JST, Timur Tabi wrote:

...

Quoted 17 lines

Quoted 10 lines

drivers/gpu/nova-core/firmware/gsp.rs pub(crate) fn new<'a, 'b>( · line 172

Quoted 9 lines

          let sigs_section = match chipset.arch() {

drivers/gpu/nova-core/firmware/gsp.rs +7 -1

-            Architecture::Ampere => ".fwsignature_ga10x",
+            Architecture::Turing | Architecture::Ampere =>
+                if chipset > Chipset::GA100 {
+                    ".fwsignature_ga10x"
+                } else {
+                    // GA100 uses the same firmware as Turing
+                    ".fwsignature_tu10x"
+                },

Quoted 3 lines

Following up the point I raised on patch 3, this could be another site where we use a potential Chipset::has_bootloader_thing or any more fittingly named method.

Ah right, that's unrelated. In this case can we just express the exception as follows:

Quoted 4 lines

     let sigs_section = match chipset.arch() {
         Architecture::Turing => ".fwsignature_tu10x",
         // GA100 uses the same firmware name as Turing
         Architecture::Ampere if chipset > Chipset::GA100 => ".fwsignature_tu10x",

I think that should be:

         Architecture::Ampere if chipset == Chipset::GA100 => ".fwsignature_tu10x",

On Sat Nov 15, 2025 at 8:30 AM JST, Timur Tabi wrote:
<snip>

drivers/gpu/nova-core/regs.rs pub(crate) fn mem_scrubbing_done(self) -> bool { · line 302

Quoted 13 lines

drivers/gpu/nova-core/regs.rs +9 -0

@@ -290,6 +291,15 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
      16:16   set_dmtag as u8;
  });
 
+impl NV_PFALCON_FALCON_DMATRFCMD {
+    /// Programs the 'imem' and 'sec' fields for the given FalconMem
+    pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
+        self
+            .set_imem(mem != FalconMem::Dmem)
+            .set_sec(if mem == FalconMem::ImemSec { 1 } else { 0 })
+    }
+}
+

After merging ImemSec and ImemNs into a single enum, you can change this code into:

     self.set_imem(matches!(mem, FalconMem::Imem { .. }))
         .set_sec(if matches!(mem, FalconMem::Imem { secure: true }) {

             1
         } else {
             0
         })

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/regs.rs | 51 +++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

drivers/gpu/nova-core/regs.rs +51 -0

index f79c7fdae6d9..c5389db1d98d 100644
@@ -223,6 +223,10 @@ pub(crate) fn vga_workspace_addr(self) -> Option<u64> {
     6:6     swgen0 as bool;
 });
 
+register!(NV_PFALCON_FALCON_IRQMCLR @ PFalconBase[0x00000014] {
+    31:0    value as u32;
+});
+
 register!(NV_PFALCON_FALCON_MAILBOX0 @ PFalconBase[0x00000040] {
     31:0    value as u32;
 });

@@ -231,6 +235,13 @@ pub(crate) fn vga_workspace_addr(self) -> Option<u64> {
     31:0    value as u32;
 });
 
+register!(NV_PFALCON_FALCON_ITFEN @ PFalconBase[0x00000048] {
+    0:0     ctxen as bool;
+    1:1     mthden as bool;
+    2:2     postwr as bool;
+    4:4     secwl_cpuctl_alias as bool;
+});
+
 // Used to store version information about the firmware running
 // on the Falcon processor.
 register!(NV_PFALCON_FALCON_OS @ PFalconBase[0x00000080] {

@@ -272,6 +283,13 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
     7:7     secure_stat as bool;
 });
 
+impl NV_PFALCON_FALCON_DMACTL {
+    /// Returns `true` if memory scrubbing is completed.
+    pub(crate) fn mem_scrubbing_done(self) -> bool {
+        !self.dmem_scrubbing() && !self.imem_scrubbing()
+    }
+}
+
 register!(NV_PFALCON_FALCON_DMATRFBASE @ PFalconBase[0x00000110] {
     31:0    base as u32;
 });

@@ -318,6 +336,33 @@ pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
     1:1     startcpu as bool;
 });
 
+register!(NV_PFALCON2_FALCON_CMEMBASE @ PFalcon2Base[0x00000160] {
+    31:18     value as u16;
+});
+
+register!(NV_PFALCON_FALCON_IMEMC @ PFalconBase[0x00000180[4; 16]] {
+    15:0      offs as u16;
+    24:24     aincw as bool;
+    28:28     secure as bool;
+});
+
+register!(NV_PFALCON_FALCON_IMEMD @ PFalconBase[0x00000184[4; 16]] {
+    31:0      data as u32;
+});
+
+register!(NV_PFALCON_FALCON_IMEMT @ PFalconBase[0x00000188[4; 16]] {
+    15:0      tag as u16;
+});
+
+register!(NV_PFALCON_FALCON_DMEMC @ PFalconBase[0x000001c0[8; 8]] {
+    15:0      offs as u16;
+    24:24     aincw as bool;
+});
+
+register!(NV_PFALCON_FALCON_DMEMD @ PFalconBase[0x000001c4[8; 8]] {
+    31:0      data as u32;
+});
+
 // Actually known as `NV_PSEC_FALCON_ENGINE` and `NV_PGSP_FALCON_ENGINE` depending on the falcon
 // instance.
 register!(NV_PFALCON_FALCON_ENGINE @ PFalconBase[0x000003c0] {

@@ -355,6 +400,12 @@ pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
 
 // PRISCV
 
+// Turing and GA100 only
+register!(NV_PRISCV_RISCV_CORE_SWITCH_RISCV_STATUS @ PFalcon2Base[0x00000240] {
+    0:0     active_stat as bool;
+});
+
+// GA102 and later
 register!(NV_PRISCV_RISCV_CPUCTL @ PFalcon2Base[0x00000388] {
     0:0     halted as bool;
     7:7     active_stat as bool;

Signature · 1 line

2.51.2

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Quoted 1 line · Timur Tabi

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Nit (for the patch title): these are falcon registers, we just happen to use them for booting the GSP. Also IIUC most of them also exist outside of Turing.

Quoted 6 lines · Alexandre Courbot

Quoted 1 line · Timur Tabi

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Nit (for the patch title): these are falcon registers, we just happen to use them for booting the GSP. Also IIUC most of them also exist outside of Turing.

hmmm, falcon registers are *also* GPU registers, though. You arrive here via the GPU's PCIe BAR0. So I'm not sure there is anything wrong with Timur's patch title, right?

Quoted 8 lines · John Hubbard

Quoted 6 lines · Alexandre Courbot

Quoted 1 line · Timur Tabi

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Nit (for the patch title): these are falcon registers, we just happen to use them for booting the GSP. Also IIUC most of them also exist outside of Turing.

Quoted 3 lines

hmmm, falcon registers are *also* GPU registers, though. You arrive here via the GPU's PCIe BAR0. So I'm not sure there is anything wrong with Timur's patch title, right?

I was referring to the email subject (which I should have quoted for clarity): "gpu: nova-core: add Turing boot registers"

Quoted 13 lines · Alexandre Courbot

Quoted 11 lines · John Hubbard

Quoted 6 lines · Alexandre Courbot

Quoted 1 line · Timur Tabi

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Nit (for the patch title): these are falcon registers, we just happen to use them for booting the GSP. Also IIUC most of them also exist outside of Turing.

hmmm, falcon registers are *also* GPU registers, though. You arrive here via the GPU's PCIe BAR0. So I'm not sure there is anything wrong with Timur's patch title, right?

Quoted 2 lines

I was referring to the email subject (which I should have quoted for clarity): "gpu: nova-core: add Turing boot registers"

Yes. But what's really wrong with that? You can't boot up Turing without booting up its GSP, which is accessed through registers that could reasonably be referred to as GPU boot registers.

I don't really think the patch title misleads, does it?

Quoted 21 lines · John Hubbard

Quoted 9 lines · Alexandre Courbot

Quoted 8 lines · John Hubbard

Quoted 6 lines · Alexandre Courbot

Quoted 1 line · Timur Tabi

Define some more GPU registers used to boot GSP-RM on Turing and GA100.

Nit (for the patch title): these are falcon registers, we just happen to use them for booting the GSP. Also IIUC most of them also exist outside of Turing.

Quoted 6 lines

Quoted 3 lines

hmmm, falcon registers are *also* GPU registers, though. You arrive here via the GPU's PCIe BAR0. So I'm not sure there is anything wrong with Timur's patch title, right?

I was referring to the email subject (which I should have quoted for clarity): "gpu: nova-core: add Turing boot registers"

Yes. But what's really wrong with that? You can't boot up Turing without booting up its GSP, which is accessed through registers that could reasonably be referred to as GPU boot registers.

Quoted 1 line

I don't really think the patch title misleads, does it?

I interpreted the title as "these registers exist on Turing only", but it is indeed subject to interpretation. In any case it is a non-important nit, so feel free to ignore if it parses fine.

Quoted 3 lines

I interpreted the title as "these registers exist on Turing only", but it is indeed subject to interpretation. In any case it is a non-important nit, so feel free to ignore if it parses fine.

Oh, I think I missed your point entirely. I think it's likely that these registers really only apply to Turing, actually, because we know that subsequent chips don't use them. (See: recent boot success on Ampere/Ada, without these registers.)

Timur can clarify that.

Quoted 9 lines · John Hubbard

Quoted 3 lines

I interpreted the title as "these registers exist on Turing only", but it is indeed subject to interpretation. In any case it is a non-important nit, so feel free to ignore if it parses fine.

Oh, I think I missed your point entirely. I think it's likely that these registers really only apply to Turing, actually, because we know that subsequent chips don't use them. (See: recent boot success on Ampere/Ada, without these registers.)

Quoted 1 line

Timur can clarify that.

Well, some of them exist only in Turing, and some of them are used only to boot on Turing.

Last I checked, there's no HAL way to specify registers that exist only on some architectures. Is that still the case? I've added a comments for some registers to indicate that.

Add the basic HAL for recognizing Turing GPUs. This isn't enough to support booting GSP-RM on Turing, but it's a start.

Note that GA100, which boots using the same method as Turing, is not supported yet.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/falcon/hal.rs       |  6 +-
 drivers/gpu/nova-core/falcon/hal/tu102.rs | 73 +++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/nova-core/falcon/hal/tu102.rs

drivers/gpu/nova-core/falcon/hal.rs +5 -1

index c77a1568ea96..7a6610e9d0a8 100644
@@ -13,6 +13,7 @@
 };
 
 mod ga102;
+mod tu102;
 
 /// Hardware Abstraction Layer for Falcon cores.
 ///

@@ -60,9 +61,12 @@ pub(super) fn falcon_hal<E: FalconEngine + 'static>(
     use Chipset::*;
 
     let hal = match chipset {
+        TU102 | TU104 | TU106 | TU116 | TU117 => {
+            KBox::new(tu102::Tu102::<E>::new(), GFP_KERNEL)? as KBox<dyn FalconHal<E>>
+        },
         GA102 | GA103 | GA104 | GA106 | GA107 | AD102 | AD103 | AD104 | AD106 | AD107 => {
             KBox::new(ga102::Ga102::<E>::new(), GFP_KERNEL)? as KBox<dyn FalconHal<E>>
-        }
+        },
         _ => return Err(ENOTSUPP),
     };
 

drivers/gpu/nova-core/falcon/hal/tu102.rs new file +55 -0

new file mode 100644
index 000000000000..edb8447d7263
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use core::marker::PhantomData;
+
+use kernel::io::poll::read_poll_timeout;
+use kernel::prelude::*;
+use kernel::time::Delta;
+
+use crate::driver::Bar0;
+use crate::falcon::{
+    Falcon, FalconBromParams, FalconEngine
+};
+use crate::regs;
+
+use super::FalconHal;
+
+pub(super) struct Tu102<E: FalconEngine>(PhantomData<E>);
+
+impl<E: FalconEngine> Tu102<E> {
+    pub(super) fn new() -> Self {
+        Self(PhantomData)
+    }
+}
+
+impl<E: FalconEngine> FalconHal<E> for Tu102<E> {
+    fn select_core(&self, _falcon: &Falcon<E>, _bar: &Bar0) -> Result {
+        Ok(())
+    }
+
+    fn signature_reg_fuse_version(
+        &self,
+        _falcon: &Falcon<E>,
+        _bar: &Bar0,
+        _engine_id_mask: u16,
+        _ucode_id: u8,
+    ) -> Result<u32> {
+        Ok(0)
+    }
+
+    fn program_brom(&self, _falcon: &Falcon<E>, _bar: &Bar0, _params: &FalconBromParams) -> Result {
+        Ok(())
+    }
+
+    fn is_riscv_active(&self, bar: &Bar0) -> bool {
+        let cpuctl = regs::NV_PRISCV_RISCV_CORE_SWITCH_RISCV_STATUS::read(bar, &E::ID);
+        cpuctl.active_stat()
+    }
+
+    fn reset_wait_mem_scrubbing(&self, bar: &Bar0) -> Result {
+        read_poll_timeout(
+            || Ok(regs::NV_PFALCON_FALCON_DMACTL::read(bar, &E::ID)),
+            |r| r.mem_scrubbing_done(),
+            Delta::ZERO,
+            Delta::from_millis(10),
+        )

The FRTS firmware in Turing and GA100 VBIOS has an older header format (v2 instead of v3). To support both v2 and v3 at runtime, add the FalconUCodeDescV2 struct, and update code that references the FalconUCodeDescV3 directly with a FalconUCodeDesc enum that encapsulates both.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/firmware.rs       | 108 +++++++++++++++++++++++-
 drivers/gpu/nova-core/firmware/fwsec.rs |  72 ++++++++++------
 drivers/gpu/nova-core/vbios.rs          |  74 ++++++++++------
 3 files changed, 202 insertions(+), 52 deletions(-)

drivers/gpu/nova-core/firmware.rs +49 -1

index 2d2008b33fb4..5ca5bf1fb610 100644
@@ -43,6 +43,43 @@ fn request_firmware(
         .and_then(|path| firmware::Firmware::request(&path, dev))
 }
 
+/// Structure used to describe some firmwares, notably FWSEC-FRTS.
+#[repr(C)]
+#[derive(Debug, Clone)]
+pub(crate) struct FalconUCodeDescV2 {
+    /// Header defined by 'NV_BIT_FALCON_UCODE_DESC_HEADER_VDESC*' in OpenRM.
+    hdr: u32,
+    /// Stored size of the ucode after the header, compressed or uncompressed
+    stored_size: u32,
+    /// Uncompressed size of the ucode.  If store_size == uncompressed_size, then the ucode
+    /// is not compressed.
+    pub(crate) uncompressed_size: u32,
+    /// Code entry point
+    pub(crate) virtual_entry: u32,
+    /// Offset after the code segment at which the Application Interface Table headers are located.
+    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'.
+    pub(crate) imem_load_size: u32,
+    /// Virtual 'IMEM' address (i.e. 'tag') at which the code should start.
+    pub(crate) imem_virt_base: u32,
+    /// Virtual address of secure IMEM segment.
+    pub(crate) imem_sec_base: u32,
+    /// Size of secure IMEM segment.
+    pub(crate) imem_sec_size: u32,
+    /// Offset into stored (uncompressed) image at which DMEM begins.
+    pub(crate) dmem_offset: u32,
+    /// Base address at which to load the data segment into 'DMEM'.
+    pub(crate) dmem_phys_base: u32,
+    /// Size in bytes of the data to copy into 'DMEM'.
+    pub(crate) dmem_load_size: u32,
+    /// "Alternate" Size of data to load into IMEM.
+    pub(crate) alt_imem_load_size: u32,
+    /// "Alternate" Size of data to load into DMEM.
+    pub(crate) alt_dmem_load_size: u32,
+}
+
 /// Structure used to describe some firmwares, notably FWSEC-FRTS.
 #[repr(C)]
 #[derive(Debug, Clone)]

@@ -76,13 +113,80 @@ pub(crate) struct FalconUCodeDescV3 {
     _reserved: u16,
 }
 
-impl FalconUCodeDescV3 {
+#[derive(Debug, Clone)]
+pub(crate) enum FalconUCodeDesc {
+    V2(FalconUCodeDescV2),
+    V3(FalconUCodeDescV3),
+}
+
+impl FalconUCodeDesc {
     /// Returns the size in bytes of the header.
     pub(crate) fn size(&self) -> usize {
+        let hdr = match self {
+            FalconUCodeDesc::V2(v2) => v2.hdr,
+            FalconUCodeDesc::V3(v3) => v3.hdr,
+        };
+
         const HDR_SIZE_SHIFT: u32 = 16;

drivers/gpu/nova-core/firmware.rs +48 -1

+
+    pub(crate) fn interface_offset(&self) -> u32 {
+        match self {
+            FalconUCodeDesc::V2(v2) => v2.interface_offset,
+            FalconUCodeDesc::V3(v3) => v3.interface_offset,
+        }
+    }
+
+
+    pub(crate) fn dmem_load_size(&self) -> u32 {
+        match self {
+            FalconUCodeDesc::V2(v2) => v2.dmem_load_size,
+            FalconUCodeDesc::V3(v3) => v3.dmem_load_size,
+        }
+    }
+
+    pub(crate) fn pkc_data_offset(&self) -> u32 {
+        match self {
+            FalconUCodeDesc::V2(_v2) => 0,
+            FalconUCodeDesc::V3(v3) => v3.pkc_data_offset,
+        }
+    }
 
-        ((self.hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+    pub(crate) fn engine_id_mask(&self) -> u16 {
+        match self {
+            FalconUCodeDesc::V2(_v2) => 0,
+            FalconUCodeDesc::V3(v3) => v3.engine_id_mask,
+        }
+    }
+
+    pub(crate) fn ucode_id(&self) -> u8 {
+        match self {
+            FalconUCodeDesc::V2(_v2) => 0,
+            FalconUCodeDesc::V3(v3) => v3.ucode_id,
+        }
+    }
+
+    pub(crate) fn signature_count(&self) -> u8 {
+        match self {
+            FalconUCodeDesc::V2(_v2) => 0,
+            FalconUCodeDesc::V3(v3) => v3.signature_count,
+        }
+    }
+
+    pub(crate) fn signature_versions(&self) -> u16 {
+        match self {
+            FalconUCodeDesc::V2(_v2) => 0,
+            FalconUCodeDesc::V3(v3) => v3.signature_versions,
+        }
     }
 }
 

drivers/gpu/nova-core/firmware/fwsec.rs +47 -25

index e4009faba6c5..36ff8ed51c23 100644
@@ -40,7 +40,7 @@
         FalconLoadTarget, //
     },
     firmware::{
-        FalconUCodeDescV3,
+        FalconUCodeDesc,
         FirmwareDmaObject,
         FirmwareSignature,
         Signed,

@@ -218,38 +218,59 @@ unsafe fn transmute_mut<T: Sized + FromBytes + AsBytes>(
 /// It is responsible for e.g. carving out the WPR2 region as the first step of the GSP bootflow.
 pub(crate) struct FwsecFirmware {
     /// Descriptor of the firmware.
-    desc: FalconUCodeDescV3,
+    desc: FalconUCodeDesc,
     /// GPU-accessible DMA object containing the firmware.
     ucode: FirmwareDmaObject<Self, Signed>,
 }
 
 impl FalconLoadParams for FwsecFirmware {
     fn imem_sec_load_params(&self) -> FalconLoadTarget {
-        FalconLoadTarget {
-            src_start: 0,
-            dst_start: self.desc.imem_phys_base,
-            len: self.desc.imem_load_size,
+        match &self.desc {
+            FalconUCodeDesc::V2(v2) => FalconLoadTarget {
+                src_start: 0,
+                dst_start: v2.imem_sec_base,
+                len: v2.imem_sec_size,
+            },
+            FalconUCodeDesc::V3(v3) => FalconLoadTarget {
+                src_start: 0,
+                dst_start: v3.imem_phys_base,
+                len: v3.imem_load_size,
+            }
         }
     }
 
     fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
-        // Only used on Turing and GA100, so return None for now
-        None
+        match &self.desc {
+            FalconUCodeDesc::V2(v2) => Some(FalconLoadTarget {
+                src_start: 0,
+                dst_start: v2.imem_phys_base,
+                len: v2.imem_load_size - v2.imem_sec_size,
+            }),
+            // Not used on V3 platforms
+            FalconUCodeDesc::V3(_v3) => None,
+        }
     }
 
     fn dmem_load_params(&self) -> FalconLoadTarget {
-        FalconLoadTarget {
-            src_start: self.desc.imem_load_size,
-            dst_start: self.desc.dmem_phys_base,
-            len: self.desc.dmem_load_size,
+        match &self.desc {
+            FalconUCodeDesc::V2(v2) => FalconLoadTarget {
+                src_start: v2.dmem_offset,
+                dst_start: v2.dmem_phys_base,
+                len: v2.dmem_load_size,
+            },
+            FalconUCodeDesc::V3(v3) => FalconLoadTarget {
+                src_start: v3.imem_load_size,
+                dst_start: v3.dmem_phys_base,
+                len: v3.dmem_load_size,
+            }
         }
     }
 
     fn brom_params(&self) -> FalconBromParams {
         FalconBromParams {
-            pkc_data_offset: self.desc.pkc_data_offset,
-            engine_id_mask: self.desc.engine_id_mask,
-            ucode_id: self.desc.ucode_id,
+            pkc_data_offset: self.desc.pkc_data_offset(),
+            engine_id_mask: self.desc.engine_id_mask(),
+            ucode_id: self.desc.ucode_id(),
         }
     }
 

@@ -273,10 +294,10 @@ impl FalconFirmware for FwsecFirmware {
 impl FirmwareDmaObject<FwsecFirmware, Unsigned> {
     fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Result<Self> {
         let desc = bios.fwsec_image().header()?;
-        let ucode = bios.fwsec_image().ucode(desc)?;
+        let ucode = bios.fwsec_image().ucode(&desc)?;
         let mut dma_object = DmaObject::from_data(dev, ucode)?;
 
-        let hdr_offset = usize::from_safe_cast(desc.imem_load_size + desc.interface_offset);
+        let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
         // SAFETY: we have exclusive access to `dma_object`.
         let hdr: &FalconAppifHdrV1 = unsafe { transmute(&dma_object, hdr_offset) }?;
 

@@ -303,7 +324,7 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
             let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
                 transmute_mut(
                     &mut dma_object,
-                    (desc.imem_load_size + dmem_base).into_safe_cast(),
+                    (desc.imem_load_size() + dmem_base).into_safe_cast(),
                 )
             }?;
 

@@ -317,7 +338,7 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
             let frts_cmd: &mut FrtsCmd = unsafe {
                 transmute_mut(
                     &mut dma_object,
-                    (desc.imem_load_size + cmd_in_buffer_offset).into_safe_cast(),
+                    (desc.imem_load_size() + cmd_in_buffer_offset).into_safe_cast(),
                 )
             }?;
 

@@ -364,11 +385,12 @@ pub(crate) fn new(
 
         // Patch signature if needed.
         let desc = bios.fwsec_image().header()?;
-        let ucode_signed = if desc.signature_count != 0 {
-            let sig_base_img = usize::from_safe_cast(desc.imem_load_size + desc.pkc_data_offset);
-            let desc_sig_versions = u32::from(desc.signature_versions);
+        let ucode_signed = if desc.signature_count() != 0 {
+            let sig_base_img =
+                usize::from_safe_cast(desc.imem_load_size() + desc.pkc_data_offset());
+            let desc_sig_versions = u32::from(desc.signature_versions());
             let reg_fuse_version =
-                falcon.signature_reg_fuse_version(bar, desc.engine_id_mask, desc.ucode_id)?;
+                falcon.signature_reg_fuse_version(bar, desc.engine_id_mask(), desc.ucode_id())?;
             dev_dbg!(
                 dev,
                 "desc_sig_versions: {:#x}, reg_fuse_version: {}\n",

@@ -402,7 +424,7 @@ pub(crate) fn new(
             dev_dbg!(dev, "patching signature with index {}\n", signature_idx);
             let signature = bios
                 .fwsec_image()
-                .sigs(desc)
+                .sigs(&desc)
                 .and_then(|sigs| sigs.get(signature_idx).ok_or(EINVAL))?;
 
             ucode_dma.patch_signature(signature, sig_base_img)?

@@ -411,7 +433,7 @@ pub(crate) fn new(
         };
 
         Ok(FwsecFirmware {
-            desc: desc.clone(),
+            desc: desc,
             ucode: ucode_signed,
         })
     }

drivers/gpu/nova-core/vbios.rs +49 -25

index abf423560ff4..860d6fb3f516 100644
@@ -19,6 +19,8 @@
     driver::Bar0,
     firmware::{
         fwsec::Bcrt30Rsa3kSignature,
+        FalconUCodeDesc,
+        FalconUCodeDescV2,
         FalconUCodeDescV3, //
     },
     num::FromSafeCast,

@@ -1004,19 +1006,10 @@ fn build(self) -> Result<FwSecBiosImage> {
 
 impl FwSecBiosImage {
     /// Get the FwSec header ([`FalconUCodeDescV3`]).
-    pub(crate) fn header(&self) -> Result<&FalconUCodeDescV3> {
+    pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
         // Get the falcon ucode offset that was found in setup_falcon_data.
         let falcon_ucode_offset = self.falcon_ucode_offset;
 
-        // Make sure the offset is within the data bounds.
-        if falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>() > self.base.data.len() {
-            dev_err!(
-                self.base.dev,
-                "fwsec-frts header not contained within BIOS bounds\n"
-            );
-            return Err(ERANGE);
-        }
-
         // Read the first 4 bytes to get the version.
         let hdr_bytes: [u8; 4] = self.base.data[falcon_ucode_offset..falcon_ucode_offset + 4]
             .try_into()

@@ -1024,33 +1017,60 @@ pub(crate) fn header(&self) -> Result<&FalconUCodeDescV3> {
         let hdr = u32::from_le_bytes(hdr_bytes);
         let ver = (hdr & 0xff00) >> 8;
 
-        if ver != 3 {
-            dev_err!(self.base.dev, "invalid fwsec firmware version: {:?}\n", ver);
-            return Err(EINVAL);
+        let hdr_size = match ver {
+            2 => core::mem::size_of::<FalconUCodeDescV2>(),
+            3 => core::mem::size_of::<FalconUCodeDescV3>(),
+            _ => {
+                dev_err!(self.base.dev, "invalid fwsec firmware version: {:?}\n", ver);
+                return Err(EINVAL);
+            }
+        };
+        // Make sure the offset is within the data bounds
+        if falcon_ucode_offset + hdr_size > self.base.data.len() {
+            dev_err!(
+                self.base.dev,
+                "fwsec-frts header not contained within BIOS bounds\n"
+            );
+            return Err(ERANGE);
         }
 
-        // Return a reference to the FalconUCodeDescV3 structure.
-        //
-        // SAFETY: We have checked that `falcon_ucode_offset + size_of::<FalconUCodeDescV3>` is
-        // within the bounds of `data`. Also, this data vector is from ROM, and the `data` field
-        // in `BiosImageBase` is immutable after construction.
-        Ok(unsafe {
+        let v2 = unsafe {
+            &*(self
+                .base
+                .data
+                .as_ptr()
+                .add(falcon_ucode_offset)
+                .cast::<FalconUCodeDescV2>())
+        };
+
+        let v3 = unsafe {
             &*(self
                 .base
                 .data
                 .as_ptr()
                 .add(falcon_ucode_offset)
                 .cast::<FalconUCodeDescV3>())
-        })
+        };
+
+        // Return a FalconUCodeDesc structure.
+        //
+        // SAFETY: We have checked that `falcon_ucode_offset + size_of::<FalconUCodeDesc>` is
+        // within the bounds of `data`. Also, this data vector is from ROM, and the `data` field
+        // in `BiosImageBase` is immutable after construction.
+        match ver {
+            2 => Ok(FalconUCodeDesc::V2(v2.clone())),
+            3 => Ok(FalconUCodeDesc::V3(v3.clone())),
+            _ => Err(EINVAL),
+        }
     }
 
     /// Get the ucode data as a byte slice
-    pub(crate) fn ucode(&self, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
+    pub(crate) fn ucode(&self, desc: &FalconUCodeDesc) -> Result<&[u8]> {
         let falcon_ucode_offset = self.falcon_ucode_offset;
 
         // The ucode data follows the descriptor.
         let ucode_data_offset = falcon_ucode_offset + desc.size();
-        let size = usize::from_safe_cast(desc.imem_load_size + desc.dmem_load_size);
+        let size = usize::from_safe_cast(desc.imem_load_size() + desc.dmem_load_size());
 
         // Get the data slice, checking bounds in a single operation.
         self.base

@@ -1066,10 +1086,14 @@ pub(crate) fn ucode(&self, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
     }
 
     /// Get the signatures as a byte slice
-    pub(crate) fn sigs(&self, desc: &FalconUCodeDescV3) -> Result<&[Bcrt30Rsa3kSignature]> {
+    pub(crate) fn sigs(&self, desc: &FalconUCodeDesc) -> Result<&[Bcrt30Rsa3kSignature]> {
+        let hdr_size = match desc {
+            FalconUCodeDesc::V2(_v2) => core::mem::size_of::<FalconUCodeDescV2>(),
+            FalconUCodeDesc::V3(_v3) => core::mem::size_of::<FalconUCodeDescV3>(),
+        };
         // The signatures data follows the descriptor.
-        let sigs_data_offset = self.falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>();
-        let sigs_count = usize::from(desc.signature_count);
+        let sigs_data_offset = self.falcon_ucode_offset + hdr_size;
+        let sigs_count = usize::from(desc.signature_count());
         let sigs_size = sigs_count * core::mem::size_of::<Bcrt30Rsa3kSignature>();
 
         // Make sure the data is within bounds.

Signature · 1 line

2.51.2

Quoted 81 lines · Timur Tabi

The FRTS firmware in Turing and GA100 VBIOS has an older header format (v2 instead of v3). To support both v2 and v3 at runtime, add the FalconUCodeDescV2 struct, and update code that references the FalconUCodeDescV3 directly with a FalconUCodeDesc enum that encapsulates both.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/firmware.rs       | 108 +++++++++++++++++++++++-
 drivers/gpu/nova-core/firmware/fwsec.rs |  72 ++++++++++------
 drivers/gpu/nova-core/vbios.rs          |  74 ++++++++++------
 3 files changed, 202 insertions(+), 52 deletions(-)

drivers/gpu/nova-core/firmware.rs +49 -1

index 2d2008b33fb4..5ca5bf1fb610 100644
@@ -43,6 +43,43 @@ fn request_firmware(
         .and_then(|path| firmware::Firmware::request(&path, dev))
 }
 
+/// Structure used to describe some firmwares, notably FWSEC-FRTS.
+#[repr(C)]
+#[derive(Debug, Clone)]
+pub(crate) struct FalconUCodeDescV2 {
+    /// Header defined by 'NV_BIT_FALCON_UCODE_DESC_HEADER_VDESC*' in OpenRM.
+    hdr: u32,
+    /// Stored size of the ucode after the header, compressed or uncompressed
+    stored_size: u32,
+    /// Uncompressed size of the ucode.  If store_size == uncompressed_size, then the ucode
+    /// is not compressed.
+    pub(crate) uncompressed_size: u32,
+    /// Code entry point
+    pub(crate) virtual_entry: u32,
+    /// Offset after the code segment at which the Application Interface Table headers are located.
+    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'.
+    pub(crate) imem_load_size: u32,
+    /// Virtual 'IMEM' address (i.e. 'tag') at which the code should start.
+    pub(crate) imem_virt_base: u32,
+    /// Virtual address of secure IMEM segment.
+    pub(crate) imem_sec_base: u32,
+    /// Size of secure IMEM segment.
+    pub(crate) imem_sec_size: u32,
+    /// Offset into stored (uncompressed) image at which DMEM begins.
+    pub(crate) dmem_offset: u32,
+    /// Base address at which to load the data segment into 'DMEM'.
+    pub(crate) dmem_phys_base: u32,
+    /// Size in bytes of the data to copy into 'DMEM'.
+    pub(crate) dmem_load_size: u32,
+    /// "Alternate" Size of data to load into IMEM.
+    pub(crate) alt_imem_load_size: u32,
+    /// "Alternate" Size of data to load into DMEM.
+    pub(crate) alt_dmem_load_size: u32,
+}
+
 /// Structure used to describe some firmwares, notably FWSEC-FRTS.
 #[repr(C)]
 #[derive(Debug, Clone)]

@@ -76,13 +113,80 @@ pub(crate) struct FalconUCodeDescV3 {
     _reserved: u16,
 }
 
-impl FalconUCodeDescV3 {
+#[derive(Debug, Clone)]
+pub(crate) enum FalconUCodeDesc {
+    V2(FalconUCodeDescV2),
+    V3(FalconUCodeDescV3),
+}
+
+impl FalconUCodeDesc {
     /// Returns the size in bytes of the header.
     pub(crate) fn size(&self) -> usize {
+        let hdr = match self {
+            FalconUCodeDesc::V2(v2) => v2.hdr,
+            FalconUCodeDesc::V3(v3) => v3.hdr,
+        };
+
         const HDR_SIZE_SHIFT: u32 = 16;

drivers/gpu/nova-core/firmware.rs pub(crate) struct FalconUCodeDescV3 { · line 140

Quoted 10 lines

drivers/gpu/nova-core/firmware.rs +9 -0

         const HDR_SIZE_MASK: u32 = 0xffff0000;
+        ((hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+    }
+
+    pub(crate) fn imem_load_size(&self) -> u32 {
+        match self {
+            FalconUCodeDesc::V2(v2) => v2.imem_load_size,
+            FalconUCodeDesc::V3(v3) => v3.imem_load_size,
+        }
+    }

This looks like the perfect use case for a trait object. You can define a trait, make both descriptors implement the trait and get rid of a lot of the match statements:

// First define trait

pub(crate) trait FalconUCodeDescriptor {
    fn imem_load_size(&self) -> u32;
    fn dmem_load_size(&self) -> u32;
    fn engine_id_mask(&self) -> u16; // V3-only field, V2 returns 0
    ...
}

// Implement trait for both versions

impl FalconUCodeDescriptor for FalconUCodeDescV2 {
    fn imem_load_size(&self) -> u32 { self.imem_load_size }
    fn dmem_load_size(&self) -> u32 { self.dmem_load_size }
    fn engine_id_mask(&self) -> u16 { 0 } // V2 doesn't have this field
    ...
}

impl FalconUCodeDescriptor for FalconUCodeDescV3 {
    fn imem_load_size(&self) -> u32 { self.imem_load_size }
    fn dmem_load_size(&self) -> u32 { self.dmem_load_size }
    fn engine_id_mask(&self) -> u16 { self.engine_id_mask }
    ...
}

// Keep the same enum. If you want to get rid of the enum, you'll need Box, // but then that requires allocation.
pub(crate) enum FalconUCodeDesc {
V2(FalconUCodeDescV2),
V3(FalconUCodeDescV3),
}

impl FalconUCodeDesc {
    // Return trait object, the only match needed.
    pub(crate) fn as_descriptor(&self) -> &dyn FalconUCodeDescriptor {
        match self {
            FalconUCodeDesc::V2(v2) => v2,
            FalconUCodeDesc::V3(v3) => v3,
        }
    }

    // delegate to trait, no match statements!
    pub(crate) fn imem_load_size(&self) -> u32 {
        self.as_descriptor().imem_load_size()
    }

    pub(crate) fn dmem_load_size(&self) -> u32 {
        self.as_descriptor().dmem_load_size()
    }
}

// Usage example - no more match statements needed!

impl FalconLoadParams for FwsecFirmware {
    fn dmem_load_params(&self) -> FalconLoadTarget {
        FalconLoadTarget {
            src_start: 0,
            dst_start: 0,
            len: self.desc.dmem_load_size(),
        }
    }
}

On principle, I tend to agree. In practice, we will probably never have more than these two variants, so we need to balance the benefit of a trait against the overhead of defining it in the first place (there are quite a few methods in there).

Trait objects come with their own complications, i.e. you need to store them on the heap if you need more than a short-lived reference - but in our case the short-lived reference should be what we need anyway.