Skip to contentSkip to conversation

gpu: nova-core: add Turing support

PATCH Superseded Timur Tabi 4 participants 24 messages Jan 3 – Jan 14

Subsystems (4)

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Timur Tabi

author
#0 lore raw

v5 cover gpu: nova-core: add Turing support

This patch set adds basic support for pre-booting GSP-RM on Turing.

There is also partial support for GA100, but it's currently not fully implemented. GA100 is considered experimental in Nouveau, and so it hasn't been tested with NovaCore either.

The latest linux-firmware.git is required because it contains the Generic Bootloader image that has not yet been propogated to distros.

Summary of changes:

1. Introduce non-secure IMEM support. For GA102 and later, only secure IMEM is used.
2. Because of non-secure IMEM, Turing booter firmware images need some of the headers parsed differently for stuff like the load target address.
3. Add support the tu10x firmware signature section in the ELF image.
4. Add several new registers used only on Turing.
5. Some functions that were considered generic Falcon operations are actually different on Turing vs GA102+, so they are moved to the HAL.
6. The FRTS FWSEC firmware in VBIOS uses a different version of the descriptor header.
7. On Turing/GA100 LIBOS args struct needs to have its 'size' field aligned to 4KB. So pad the struct to make it 4K.
8. Turing Falcons do not support DMA, so PIO is used to copy images into IMEM/DMEM.

Changes from v4:
1. Fixed various nits
2. Added enum LoadMethod to control whether fw images are uploaded via DMA or PIO
3. Added GspArgumentsAligned to ensure RMARGS is 4k-aligned.
4. Replaced FalconUCodeDesc.as_descriptor with impl Deref.

Alexandre Courbot (1):
gpu: nova-core: align LibosMemoryRegionInitArgument size to page size

Timur Tabi (10):
gpu: nova-core: rename Imem to ImemSecure
gpu: nova-core: add ImemNonSecure section infrastructure
gpu: nova-core: support header parsing on Turing/GA100
gpu: nova-core: add support for Turing/GA100 fwsignature
gpu: nova-core: add NV_PFALCON_FALCON_DMATRFCMD::with_falcon_mem()
gpu: nova-core: move some functions into the HAL
gpu: nova-core: Add basic Turing HAL
gpu: nova-core: add Falcon HAL method supports_dma()
gpu: nova-core: add FalconUCodeDescV2 support
gpu: nova-core: add PIO support for loading firmware images

 drivers/gpu/nova-core/falcon.rs           | 252 +++++++++++++++++-----
 drivers/gpu/nova-core/falcon/hal.rs       |  22 ++
 drivers/gpu/nova-core/falcon/hal/ga102.rs |  46 ++++
 drivers/gpu/nova-core/falcon/hal/tu102.rs |  84 ++++++++
 drivers/gpu/nova-core/firmware.rs         | 149 ++++++++++++-
 drivers/gpu/nova-core/firmware/booter.rs  |  43 +++-
 drivers/gpu/nova-core/firmware/fwsec.rs   | 199 ++++++++++++++---
 drivers/gpu/nova-core/firmware/gsp.rs     |   6 +-
 drivers/gpu/nova-core/gsp.rs              |   8 +-
 drivers/gpu/nova-core/gsp/boot.rs         |   6 +-
 drivers/gpu/nova-core/gsp/fw.rs           |  14 +-
 drivers/gpu/nova-core/regs.rs             |  53 +++++
 drivers/gpu/nova-core/vbios.rs            |  75 ++++---
 13 files changed, 834 insertions(+), 123 deletions(-)
 create mode 100644 drivers/gpu/nova-core/falcon/hal/tu102.rs

base-commit: 7acc70476f14661149774ab88d3fe23d83ba4249

Signature · 1 line
2.51.0
author
#1 lore raw

PATCH v5 1/11 gpu: nova-core: rename Imem to ImemSecure

Rename FalconMem::Imem to ImemSecure 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 also 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.

---
 drivers/gpu/nova-core/falcon.rs          | 20 +++++++++++++-------
 drivers/gpu/nova-core/firmware/booter.rs | 12 ++++++------
 drivers/gpu/nova-core/firmware/fwsec.rs  |  2 +-
 3 files changed, 20 insertions(+), 14 deletions(-)
drivers/gpu/nova-core/falcon.rs +13 -7
index 82c661aef594..0965cb50568b 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.
+    ImemSecure,
     /// 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;
@@ -457,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::Imem => (load_offsets.src_start, fw.dma_handle()),
+            FalconMem::ImemSecure => (load_offsets.src_start, fw.dma_handle()),
             FalconMem::Dmem => (
                 0,
                 fw.dma_handle_with_offset(load_offsets.src_start.into_safe_cast())?,
@@ -508,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::Imem)
+            .set_imem(target_mem == FalconMem::ImemSecure)
             .set_sec(if sec { 1 } else { 0 });
 
         for pos in (0..num_transfers).map(|i| i * DMA_LEN) {
@@ -543,7 +543,13 @@ 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::ImemSecure,
+            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,
Signature · 1 line
2.51.0
author
#2 lore raw

PATCH v5 2/11 gpu: nova-core: add ImemNonSecure section infrastructure

In reply to Timur Tabi

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.

---
 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(-)
drivers/gpu/nova-core/falcon.rs +17 -2
index 0965cb50568b..974abc2de649 100644
@@ -239,6 +239,8 @@ fn from(value: PeregrineCoreSelect) -> Self {
 pub(crate) enum FalconMem {
     /// Secure Instruction Memory.
     ImemSecure,
+    /// Non-Secure Instruction Memory.
+    ImemNonSecure,
     /// 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;
 
@@ -457,7 +463,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())?,
@@ -508,7 +516,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) {
@@ -537,6 +545,13 @@ 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);
+        }
+
         self.dma_reset(bar);
         regs::NV_PFALCON_FBIF_TRANSCFG::update(bar, &E::ID, 0, |v| {
             v.set_target(FalconFbifTarget::CoherentSysmem)
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,
Signature · 1 line
2.51.0
author
#3 lore raw

PATCH v5 3/11 gpu: nova-core: support header parsing on Turing/GA100

In reply to Timur Tabi

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.

---
 drivers/gpu/nova-core/firmware/booter.rs | 28 ++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)
drivers/gpu/nova-core/firmware/booter.rs +24 -4
index 1b98bb47424c..86556cee8e67 100644
@@ -356,14 +356,30 @@ 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.
+        let (imem_sec_dst_start, imem_ns_load_target) = if chipset <= Chipset::GA100 {
+            (
+                app0.offset,
+                Some(FalconLoadTarget {
+                    src_start: 0,
+                    dst_start: load_hdr.os_code_offset,
+                    len: load_hdr.os_code_size,
+                }),
+            )
+        } else {
+            (0, None)
+        };
+
         Ok(Self {
             imem_sec_load_target: FalconLoadTarget {
                 src_start: app0.offset,
-                dst_start: 0,
+                dst_start: imem_sec_dst_start,
                 len: app0.len,
             },
-            // Exists only in the booter image for Turing and GA100
-            imem_ns_load_target: None,
+            imem_ns_load_target,
             dmem_load_target: FalconLoadTarget {
                 src_start: load_hdr.os_data_offset,
                 dst_start: 0,
@@ -393,7 +409,11 @@ fn brom_params(&self) -> FalconBromParams {
     }
 
     fn boot_addr(&self) -> u32 {
-        self.imem_sec_load_target.src_start
+        if let Some(ns_target) = &self.imem_ns_load_target {
+            ns_target.dst_start
+        } else {
+            self.imem_sec_load_target.src_start
+        }
     }
 }
 
Signature · 1 line
2.51.0
author
#4 lore raw

PATCH v5 4/11 gpu: nova-core: add support for Turing/GA100 fwsignature

In reply to Timur Tabi

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

---
 drivers/gpu/nova-core/firmware/gsp.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/gpu/nova-core/firmware/gsp.rs +5 -1
index da97814cf859..c463b60799a4 100644
@@ -217,9 +217,13 @@ pub(crate) fn new<'a>(
                 size,
                 signatures: {
                     let sigs_section = match chipset.arch() {
+                        Architecture::Turing if chipset == Chipset::TU116 || chipset == Chipset::TU117 =>
+                            ".fwsignature_tu11x",
+                        Architecture::Turing => ".fwsignature_tu10x",
+                        // GA100 uses the same firmware as Turing
+                        Architecture::Ampere if chipset == Chipset::GA100 => ".fwsignature_tu10x",
                         Architecture::Ampere => ".fwsignature_ga10x",
                         Architecture::Ada => ".fwsignature_ad10x",
-                        _ => return Err(ENOTSUPP),
                     };
 
                     elf::elf64_section(firmware.data(), sigs_section)
Signature · 1 line
2.51.0
author
#5 lore raw

PATCH v5 5/11 gpu: nova-core: add NV_PFALCON_FALCON_DMATRFCMD::with_falcon_mem()

In reply to Timur Tabi

The with_falcon_mem() method initializes the 'imem' and 'sec' fields of the NV_PFALCON_FALCON_DMATRFCMD register based on the value of the FalconMem type.

---
 drivers/gpu/nova-core/falcon.rs | 14 +++-----------
 drivers/gpu/nova-core/regs.rs   |  9 +++++++++
 2 files changed, 12 insertions(+), 11 deletions(-)
drivers/gpu/nova-core/falcon.rs +3 -11
index 974abc2de649..fd972ec19a67 100644
@@ -454,7 +454,6 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
         fw: &F,
         target_mem: FalconMem,
         load_offsets: FalconLoadTarget,
-        sec: bool,
     ) -> Result {
         const DMA_LEN: u32 = 256;
 
@@ -516,8 +515,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::Dmem)
-            .set_sec(if sec { 1 } else { 0 });
+            .with_falcon_mem(target_mem);
 
         for pos in (0..num_transfers).map(|i| i * DMA_LEN) {
             // Perform a transfer of size `DMA_LEN`.
@@ -558,14 +556,8 @@ 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::ImemSecure,
-            fw.imem_sec_load_params(),
-            true,
-        )?;
-        self.dma_wr(bar, fw, FalconMem::Dmem, fw.dmem_load_params(), true)?;
+        self.dma_wr(bar, fw, FalconMem::ImemSecure, fw.imem_sec_load_params())?;
+        self.dma_wr(bar, fw, FalconMem::Dmem, fw.dmem_load_params())?;
 
         self.hal.program_brom(self, bar, &fw.brom_params())?;
 
drivers/gpu/nova-core/regs.rs +9 -0
index 82cc6c0790e5..b8ddfe2e5ae7 100644
@@ -16,6 +16,7 @@
         FalconCoreRevSubversion,
         FalconFbifMemType,
         FalconFbifTarget,
+        FalconMem,
         FalconModSelAlgo,
         FalconSecurityModel,
         PFalcon2Base,
@@ -325,6 +326,14 @@ 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::ImemSecure { 1 } else { 0 })
+    }
+}
+
 register!(NV_PFALCON_FALCON_DMATRFFBOFFS @ PFalconBase[0x0000011c] {
     31:0    offs as u32;
 });
Signature · 1 line
2.51.0
author
#6 lore raw

PATCH v5 6/11 gpu: nova-core: move some functions into the HAL

In reply to Timur Tabi

A few Falcon methods are actually GPU-specific, so move them into the HAL.

---
 drivers/gpu/nova-core/falcon.rs           | 45 ++---------------------
 drivers/gpu/nova-core/falcon/hal.rs       | 10 +++++
 drivers/gpu/nova-core/falcon/hal/ga102.rs | 41 +++++++++++++++++++++
 3 files changed, 54 insertions(+), 42 deletions(-)
drivers/gpu/nova-core/falcon.rs +3 -42
index fd972ec19a67..1669a7d19b1b 100644
@@ -13,7 +13,6 @@
     prelude::*,
     sync::aref::ARef,
     time::{
-        delay::fsleep,
         Delta, //
     },
 };
@@ -394,48 +393,11 @@ pub(crate) fn dma_reset(&self, bar: &Bar0) {
         regs::NV_PFALCON_FALCON_DMACTL::default().write(bar, &E::ID);
     }
 
-    /// Wait for memory scrubbing to complete.
-    fn reset_wait_mem_scrubbing(&self, bar: &Bar0) -> Result {
-        // TIMEOUT: memory scrubbing should complete in less than 20ms.
-        read_poll_timeout(
-            || Ok(regs::NV_PFALCON_FALCON_HWCFG2::read(bar, &E::ID)),
-            |r| r.mem_scrubbing_done(),
-            Delta::ZERO,
-            Delta::from_millis(20),
-        )
-        .map(|_| ())
-    }
-
-    /// Reset the falcon engine.
-    fn reset_eng(&self, bar: &Bar0) -> Result {
-        let _ = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, &E::ID);
-
-        // According to OpenRM's `kflcnPreResetWait_GA102` documentation, HW sometimes does not set
-        // RESET_READY so a non-failing timeout is used.
-        let _ = read_poll_timeout(
-            || Ok(regs::NV_PFALCON_FALCON_HWCFG2::read(bar, &E::ID)),
-            |r| r.reset_ready(),
-            Delta::ZERO,
-            Delta::from_micros(150),
-        );
-
-        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| v.set_reset(true));
-
-        // TIMEOUT: falcon engine should not take more than 10us to reset.
-        fsleep(Delta::from_micros(10));
-
-        regs::NV_PFALCON_FALCON_ENGINE::update(bar, &E::ID, |v| v.set_reset(false));
-
-        self.reset_wait_mem_scrubbing(bar)?;
-
-        Ok(())
-    }
-
     /// Reset the controller, select the falcon core, and wait for memory scrubbing to complete.
     pub(crate) fn reset(&self, bar: &Bar0) -> Result {
-        self.reset_eng(bar)?;
+        self.hal.reset_eng(bar)?;
         self.hal.select_core(self, bar)?;
-        self.reset_wait_mem_scrubbing(bar)?;
+        self.hal.reset_wait_mem_scrubbing(bar)?;
 
         regs::NV_PFALCON_FALCON_RM::default()
             .set_value(regs::NV_PMC_BOOT_0::read(bar).into())
@@ -664,8 +626,7 @@ pub(crate) fn signature_reg_fuse_version(
     ///
     /// Returns `true` if the RISC-V core is active, `false` otherwise.
     pub(crate) fn is_riscv_active(&self, bar: &Bar0) -> bool {
-        let cpuctl = regs::NV_PRISCV_RISCV_CPUCTL::read(bar, &E::ID);
-        cpuctl.active_stat()
+        self.hal.is_riscv_active(bar)
     }
 
     /// Write the application version to the OS register.
drivers/gpu/nova-core/falcon/hal.rs +10 -0
index 8dc56a28ad65..c77a1568ea96 100644
@@ -37,6 +37,16 @@ fn signature_reg_fuse_version(
 
     /// Program the boot ROM registers prior to starting a secure firmware.
     fn program_brom(&self, falcon: &Falcon<E>, bar: &Bar0, params: &FalconBromParams) -> Result;
+
+    /// Check if the RISC-V core is active.
+    /// Returns `true` if the RISC-V core is active, `false` otherwise.
+    fn is_riscv_active(&self, bar: &Bar0) -> bool;
+
+    /// Wait for memory scrubbing to complete.
+    fn reset_wait_mem_scrubbing(&self, bar: &Bar0) -> Result;
+
+    /// Reset the falcon engine.
+    fn reset_eng(&self, bar: &Bar0) -> Result;
 }
 
 /// Returns a boxed falcon HAL adequate for `chipset`.
drivers/gpu/nova-core/falcon/hal/ga102.rs +41 -0

51 lines not on this page · raw message

Signature · 1 line
2.51.0
author
#7 lore raw

PATCH v5 7/11 gpu: nova-core: Add basic Turing HAL

In reply to Timur Tabi

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.

---
 drivers/gpu/nova-core/falcon/hal.rs       |  4 ++
 drivers/gpu/nova-core/falcon/hal/tu102.rs | 79 +++++++++++++++++++++++
 drivers/gpu/nova-core/regs.rs             | 14 ++++
 3 files changed, 97 insertions(+)
 create mode 100644 drivers/gpu/nova-core/falcon/hal/tu102.rs
drivers/gpu/nova-core/falcon/hal.rs +4 -0
index c77a1568ea96..c886ba03d1f6 100644
@@ -13,6 +13,7 @@
 };
 
 mod ga102;
+mod tu102;
 
 /// Hardware Abstraction Layer for Falcon cores.
 ///
@@ -60,6 +61,9 @@ 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>>
         }
drivers/gpu/nova-core/falcon/hal/tu102.rs new file +79 -0

79 lines not on this page · raw message

drivers/gpu/nova-core/regs.rs +14 -0
index b8ddfe2e5ae7..cd7b7aa6fc2a 100644
@@ -307,6 +307,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;
 });
@@ -389,6 +396,13 @@ pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
 
 // PRISCV
 
+// RISC-V status register for debug (Turing and GA100 only).
+// Reflects current RISC-V core status.
+register!(NV_PRISCV_RISCV_CORE_SWITCH_RISCV_STATUS @ PFalcon2Base[0x00000240] {
+    0:0     active_stat as bool, "RISC-V core active/inactive status";
+});
+
+// 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.0
author
#8 lore raw

PATCH v5 8/11 gpu: nova-core: add Falcon HAL method supports_dma()

In reply to Timur Tabi

Some GPUs do not support using DMA to transfer code/data from system memory to Falcon memory, and instead must use programmed I/O (PIO). Add a function to the Falcon HAL to indicate whether a given GPU's Falcons support DMA for this purpose.

---
 drivers/gpu/nova-core/falcon.rs           | 7 +++++++
 drivers/gpu/nova-core/falcon/hal.rs       | 3 +++
 drivers/gpu/nova-core/falcon/hal/ga102.rs | 4 ++++
 drivers/gpu/nova-core/falcon/hal/tu102.rs | 4 ++++
 4 files changed, 18 insertions(+)
drivers/gpu/nova-core/falcon.rs +7 -0
index 1669a7d19b1b..744f513da9cd 100644
@@ -629,6 +629,13 @@ pub(crate) fn is_riscv_active(&self, bar: &Bar0) -> bool {
         self.hal.is_riscv_active(bar)
     }
 
+    /// Check if this Falcon supports DMA for loading firmware.
+    ///
+    /// Returns `true` if DMA is supported, `false` if PIO must be used instead.
+    pub(crate) fn supports_dma(&self) -> bool {
+        self.hal.supports_dma()
+    }
+
     /// Write the application version to the OS register.
     pub(crate) fn write_os_version(&self, bar: &Bar0, app_version: u32) {
         regs::NV_PFALCON_FALCON_OS::default()
drivers/gpu/nova-core/falcon/hal.rs +3 -0
index c886ba03d1f6..49501aa6ff7f 100644
@@ -48,6 +48,9 @@ fn signature_reg_fuse_version(
 
     /// Reset the falcon engine.
     fn reset_eng(&self, bar: &Bar0) -> Result;
+
+    /// Returns true of this Falcon supports DMA transfer from system memory to Falcon memory
+    fn supports_dma(&self) -> bool;
 }
 
 /// Returns a boxed falcon HAL adequate for `chipset`.
drivers/gpu/nova-core/falcon/hal/ga102.rs +4 -0
index 5a263dc37d63..b2857293e653 100644
@@ -158,4 +158,8 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
 
         Ok(())
     }
+
+    fn supports_dma(&self) -> bool {
+        true
+    }
 }
drivers/gpu/nova-core/falcon/hal/tu102.rs +4 -0
index 586d5dc6b417..28dc03db8240 100644
@@ -76,4 +76,8 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
 
         Ok(())
     }
+
+    fn supports_dma(&self) -> bool {
+        false
+    }
 }
Signature · 1 line
2.51.0
Reviewed-by on v5 8/11
#9 lore raw

Re: [PATCH v5 08/11] gpu: nova-core: add Falcon HAL method supports_dma()

Quoted 5 lines · Timur Tabi

Some GPUs do not support using DMA to transfer code/data from system memory to Falcon memory, and instead must use programmed I/O (PIO). Add a function to the Falcon HAL to indicate whether a given GPU's Falcons support DMA for this purpose.

Quoted 7 lines
---
 drivers/gpu/nova-core/falcon.rs           | 7 +++++++
 drivers/gpu/nova-core/falcon/hal.rs       | 3 +++
 drivers/gpu/nova-core/falcon/hal/ga102.rs | 4 ++++
 drivers/gpu/nova-core/falcon/hal/tu102.rs | 4 ++++
 4 files changed, 18 insertions(+)

Seems simple and clear. I know there was some exploration of alternative ways to structure this, and I haven't read the following patches yet, so maybe I'll eventually notice an opportunity for improving the Rust HAL code here. But at this point, it looks good to me, so:

thanks,

Signature · 1 line
John Hubbard
Quoted 59 lines
drivers/gpu/nova-core/falcon.rs +7 -0
index 1669a7d19b1b..744f513da9cd 100644
@@ -629,6 +629,13 @@ pub(crate) fn is_riscv_active(&self, bar: &Bar0) -> bool {
         self.hal.is_riscv_active(bar)
     }
 
+    /// Check if this Falcon supports DMA for loading firmware.
+    ///
+    /// Returns `true` if DMA is supported, `false` if PIO must be used instead.
+    pub(crate) fn supports_dma(&self) -> bool {
+        self.hal.supports_dma()
+    }
+
     /// Write the application version to the OS register.
     pub(crate) fn write_os_version(&self, bar: &Bar0, app_version: u32) {
         regs::NV_PFALCON_FALCON_OS::default()
drivers/gpu/nova-core/falcon/hal.rs +3 -0
index c886ba03d1f6..49501aa6ff7f 100644
@@ -48,6 +48,9 @@ fn signature_reg_fuse_version(
 
     /// Reset the falcon engine.
     fn reset_eng(&self, bar: &Bar0) -> Result;
+
+    /// Returns true of this Falcon supports DMA transfer from system memory to Falcon memory
+    fn supports_dma(&self) -> bool;
 }
 
 /// Returns a boxed falcon HAL adequate for `chipset`.
drivers/gpu/nova-core/falcon/hal/ga102.rs +4 -0
index 5a263dc37d63..b2857293e653 100644
@@ -158,4 +158,8 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
 
         Ok(())
     }
+
+    fn supports_dma(&self) -> bool {
+        true
+    }
 }
drivers/gpu/nova-core/falcon/hal/tu102.rs +4 -0
index 586d5dc6b417..28dc03db8240 100644
@@ -76,4 +76,8 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
 
         Ok(())
     }
+
+    fn supports_dma(&self) -> bool {
+        false
+    }
 }
author
#10 lore raw

PATCH v5 9/11 gpu: nova-core: add FalconUCodeDescV2 support

In reply to 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.

---
 drivers/gpu/nova-core/firmware.rs       | 149 +++++++++++++++++++++++-
 drivers/gpu/nova-core/firmware/fwsec.rs |  72 ++++++++----
 drivers/gpu/nova-core/vbios.rs          |  75 +++++++-----
 3 files changed, 237 insertions(+), 59 deletions(-)
drivers/gpu/nova-core/firmware.rs +146 -3
index 2d2008b33fb4..9f0ad02fbe22 100644
@@ -4,6 +4,7 @@
 //! to be loaded into a given execution unit.
 
 use core::marker::PhantomData;
+use core::ops::Deref;
 
 use kernel::{
     device,
@@ -43,6 +44,46 @@ 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,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for FalconUCodeDescV2 {}
+
 /// Structure used to describe some firmwares, notably FWSEC-FRTS.
 #[repr(C)]
 #[derive(Debug, Clone)]
@@ -76,13 +117,115 @@ pub(crate) struct FalconUCodeDescV3 {
     _reserved: u16,
 }
 
-impl FalconUCodeDescV3 {
+// SAFETY: all bit patterns are valid for this type, and it doesn't use
+// interior mutability.
+unsafe impl FromBytes for FalconUCodeDescV3 {}
+
+/// Enum wrapping the different versions of Falcon microcode descriptors.
+///
+/// This allows handling both V2 and V3 descriptor formats through a
+/// unified type, providing version-agnostic access to firmware metadata
+/// via the [`FalconUCodeDescriptor`] trait.
+#[derive(Debug, Clone)]
+pub(crate) enum FalconUCodeDesc {
+    V2(FalconUCodeDescV2),
+    V3(FalconUCodeDescV3),
+}
+
+impl Deref for FalconUCodeDesc {
+    type Target = dyn FalconUCodeDescriptor;
+
+    fn deref(&self) -> &Self::Target {
+        match self {
+            FalconUCodeDesc::V2(v2) => v2,
+            FalconUCodeDesc::V3(v3) => v3,
+        }
+    }
+}
+
+/// Trait providing a common interface for accessing Falcon microcode descriptor fields.
+///
+/// This trait abstracts over the different descriptor versions ([`FalconUCodeDescV2`] and
+/// [`FalconUCodeDescV3`]), allowing code to work with firmware metadata without needing to
+/// know the specific descriptor version. Fields not present return zero.
+pub(crate) trait FalconUCodeDescriptor {
+    fn hdr(&self) -> u32;
+    fn imem_load_size(&self) -> u32;
+    fn interface_offset(&self) -> u32;
+    fn dmem_load_size(&self) -> u32;
+    fn pkc_data_offset(&self) -> u32;
+    fn engine_id_mask(&self) -> u16;
+    fn ucode_id(&self) -> u8;
+    fn signature_count(&self) -> u8;
+    fn signature_versions(&self) -> u16;
+
     /// Returns the size in bytes of the header.
-    pub(crate) fn size(&self) -> usize {
+    fn size(&self) -> usize {
+        let hdr = self.hdr();
+
         const HDR_SIZE_SHIFT: u32 = 16;
         const HDR_SIZE_MASK: u32 = 0xffff0000;
+        ((hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+    }
+}
+
+impl FalconUCodeDescriptor for FalconUCodeDescV2 {
+    fn hdr(&self) -> u32 {
+        self.hdr
+    }
+    fn imem_load_size(&self) -> u32 {
+        self.imem_load_size
+    }
+    fn interface_offset(&self) -> u32 {
+        self.interface_offset
+    }
+    fn dmem_load_size(&self) -> u32 {
+        self.dmem_load_size
+    }
+    fn pkc_data_offset(&self) -> u32 {
+        0
+    }
+    fn engine_id_mask(&self) -> u16 {
+        0
+    }
+    fn ucode_id(&self) -> u8 {
+        0
+    }
+    fn signature_count(&self) -> u8 {
+        0
+    }
+    fn signature_versions(&self) -> u16 {
+        0
+    }
+}
 
-        ((self.hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+impl FalconUCodeDescriptor for FalconUCodeDescV3 {
+    fn hdr(&self) -> u32 {
+        self.hdr
+    }
+    fn imem_load_size(&self) -> u32 {
+        self.imem_load_size
+    }
+    fn interface_offset(&self) -> u32 {
+        self.interface_offset
+    }
+    fn dmem_load_size(&self) -> u32 {
+        self.dmem_load_size
+    }
+    fn pkc_data_offset(&self) -> u32 {
+        self.pkc_data_offset
+    }
+    fn engine_id_mask(&self) -> u16 {
+        self.engine_id_mask
+    }
+    fn ucode_id(&self) -> u8 {
+        self.ucode_id
+    }
+    fn signature_count(&self) -> u8 {
+        self.signature_count
+    }
+    fn signature_versions(&self) -> u16 {
+        self.signature_versions
     }
 }
 
drivers/gpu/nova-core/firmware/fwsec.rs +47 -25

141 lines not on this page · raw message

drivers/gpu/nova-core/vbios.rs +44 -31

112 lines not on this page · raw message

Signature · 1 line
2.51.0
#11 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

On 1/2/26 8:59 PM, Timur Tabi wrote:
...

Review

Quoted 10 lines
(quoted diff) +10 -0
+impl Deref for FalconUCodeDesc {
+    type Target = dyn FalconUCodeDescriptor;
+
+    fn deref(&self) -> &Self::Target {
+        match self {
+            FalconUCodeDesc::V2(v2) => v2,
+            FalconUCodeDesc::V3(v3) => v3,
+        }
+    }
+}

OK, I really don't want to do it this way, because this approach mixes compile-time (explicit matching) and runtime (vtable) dispath, which is odd and will surprise Rust readers.

I looked at Alex Courbot's feedback on the previous v4 patchset for this patch, but he focused on things that I don't want to care about, which is "what if we add more ucode versions". Because that won't happen--we are done creating hardware/firmware for these older GPUs.

So what we want is a nice clear, directly readable implementation that is *reasonably* Rust idiomatic. And I think that's available via:

1) Use direction enum methods, and

2) Use Option<> instead of zero sentinel values.

I've implemented these, and runtime tested the combined approach on my TU117 board, and am including a diff below that can be applied on top of your patchset.

It may have its own imperfections, but this is *approximately* what I'd prefer (below). I believe that it strikes the right balance between simplicity and Rust idiomatic code--and it's not any larger than what you started with.

And it won't cost you any time to implement, since this already works:

drivers/gpu/nova-core/firmware.rs +66 -83
index 9f0ad02fbe22..c499491c25ab 100644
@@ -4,7 +4,6 @@
 //! to be loaded into a given execution unit.
 
 use core::marker::PhantomData;
-use core::ops::Deref;
 
 use kernel::{
     device,
@@ -124,108 +123,92 @@ unsafe impl FromBytes for FalconUCodeDescV3 {}
 /// Enum wrapping the different versions of Falcon microcode descriptors.
 ///
 /// This allows handling both V2 and V3 descriptor formats through a
-/// unified type, providing version-agnostic access to firmware metadata
-/// via the [`FalconUCodeDescriptor`] trait.
+/// unified type, providing version-agnostic access to common firmware
+/// metadata. Fields not present in a particular version return zero.
 #[derive(Debug, Clone)]
 pub(crate) enum FalconUCodeDesc {
     V2(FalconUCodeDescV2),
     V3(FalconUCodeDescV3),
 }
 
-impl Deref for FalconUCodeDesc {
-    type Target = dyn FalconUCodeDescriptor;
-
-    fn deref(&self) -> &Self::Target {
+impl FalconUCodeDesc {
+    /// Returns the header word.
+    pub(crate) fn hdr(&self) -> u32 {
         match self {
-            FalconUCodeDesc::V2(v2) => v2,
-            FalconUCodeDesc::V3(v3) => v3,
+            Self::V2(v2) => v2.hdr,
+            Self::V3(v3) => v3.hdr,
         }
     }
-}
-
-/// Trait providing a common interface for accessing Falcon microcode descriptor fields.
-///
-/// This trait abstracts over the different descriptor versions ([`FalconUCodeDescV2`] and
-/// [`FalconUCodeDescV3`]), allowing code to work with firmware metadata without needing to
-/// know the specific descriptor version. Fields not present return zero.
-pub(crate) trait FalconUCodeDescriptor {
-    fn hdr(&self) -> u32;
-    fn imem_load_size(&self) -> u32;
-    fn interface_offset(&self) -> u32;
-    fn dmem_load_size(&self) -> u32;
-    fn pkc_data_offset(&self) -> u32;
-    fn engine_id_mask(&self) -> u16;
-    fn ucode_id(&self) -> u8;
-    fn signature_count(&self) -> u8;
-    fn signature_versions(&self) -> u16;
-
-    /// Returns the size in bytes of the header.
-    fn size(&self) -> usize {
-        let hdr = self.hdr();
 
-        const HDR_SIZE_SHIFT: u32 = 16;
-        const HDR_SIZE_MASK: u32 = 0xffff0000;
-        ((hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+    /// Returns the IMEM load size in bytes.
+    pub(crate) fn imem_load_size(&self) -> u32 {
+        match self {
+            Self::V2(v2) => v2.imem_load_size,
+            Self::V3(v3) => v3.imem_load_size,
+        }
     }
-}
 
-impl FalconUCodeDescriptor for FalconUCodeDescV2 {
-    fn hdr(&self) -> u32 {
-        self.hdr
-    }
-    fn imem_load_size(&self) -> u32 {
-        self.imem_load_size
-    }
-    fn interface_offset(&self) -> u32 {
-        self.interface_offset
-    }
-    fn dmem_load_size(&self) -> u32 {
-        self.dmem_load_size
-    }
-    fn pkc_data_offset(&self) -> u32 {
-        0
-    }
-    fn engine_id_mask(&self) -> u16 {
-        0
-    }
-    fn ucode_id(&self) -> u8 {
-        0
-    }
-    fn signature_count(&self) -> u8 {
-        0
-    }
-    fn signature_versions(&self) -> u16 {
-        0
+    /// Returns the interface offset.
+    pub(crate) fn interface_offset(&self) -> u32 {
+        match self {
+            Self::V2(v2) => v2.interface_offset,
+            Self::V3(v3) => v3.interface_offset,
+        }
     }
-}
 
-impl FalconUCodeDescriptor for FalconUCodeDescV3 {
-    fn hdr(&self) -> u32 {
-        self.hdr
-    }
-    fn imem_load_size(&self) -> u32 {
-        self.imem_load_size
-    }
-    fn interface_offset(&self) -> u32 {
-        self.interface_offset
+    /// Returns the DMEM load size in bytes.
+    pub(crate) fn dmem_load_size(&self) -> u32 {
+        match self {
+            Self::V2(v2) => v2.dmem_load_size,
+            Self::V3(v3) => v3.dmem_load_size,
+        }
     }
-    fn dmem_load_size(&self) -> u32 {
-        self.dmem_load_size
+
+    /// Returns the PKC data offset (V3 only).
+    pub(crate) fn pkc_data_offset(&self) -> Option<u32> {
+        match self {
+            Self::V2(_) => None,
+            Self::V3(v3) => Some(v3.pkc_data_offset),
+        }
     }
-    fn pkc_data_offset(&self) -> u32 {
-        self.pkc_data_offset
+
+    /// Returns the engine ID mask (V3 only).
+    pub(crate) fn engine_id_mask(&self) -> Option<u16> {
+        match self {
+            Self::V2(_) => None,
+            Self::V3(v3) => Some(v3.engine_id_mask),
+        }
     }
-    fn engine_id_mask(&self) -> u16 {
-        self.engine_id_mask
+
+    /// Returns the ucode ID (V3 only).
+    pub(crate) fn ucode_id(&self) -> Option<u8> {
+        match self {
+            Self::V2(_) => None,
+            Self::V3(v3) => Some(v3.ucode_id),
+        }
     }
-    fn ucode_id(&self) -> u8 {
-        self.ucode_id
+
+    /// Returns the signature count (V3 only).
+    pub(crate) fn signature_count(&self) -> Option<u8> {
+        match self {
+            Self::V2(_) => None,
+            Self::V3(v3) => Some(v3.signature_count),
+        }
     }
-    fn signature_count(&self) -> u8 {
-        self.signature_count
+
+    /// Returns the signature versions bitmask (V3 only).
+    pub(crate) fn signature_versions(&self) -> Option<u16> {
+        match self {
+            Self::V2(_) => None,
+            Self::V3(v3) => Some(v3.signature_versions),
+        }
     }
-    fn signature_versions(&self) -> u16 {
-        self.signature_versions
+
+    /// Returns the size in bytes of the descriptor header.
+    pub(crate) fn size(&self) -> usize {
+        const HDR_SIZE_SHIFT: u32 = 16;
+        const HDR_SIZE_MASK: u32 = 0xffff0000;
+        ((self.hdr() & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
     }
 }
 
drivers/gpu/nova-core/firmware/fwsec.rs +59 -45

121 lines not on this page · raw message

drivers/gpu/nova-core/vbios.rs +5 -2
index 12256e5c8dc5..349b994fef92 100644
@@ -1074,7 +1074,9 @@ pub(crate) fn ucode(&self, desc: &FalconUCodeDesc) -> Result<&[u8]> {
             })
     }
 
-    /// Get the signatures as a byte slice
+    /// Get the signatures as a byte slice.
+    ///
+    /// For V2 descriptors (which have no signature metadata), this returns an empty slice.
     pub(crate) fn sigs(&self, desc: &FalconUCodeDesc) -> Result<&[Bcrt30Rsa3kSignature]> {
         let hdr_size = match desc {
             FalconUCodeDesc::V2(_v2) => core::mem::size_of::<FalconUCodeDescV2>(),
@@ -1082,7 +1084,8 @@ pub(crate) fn sigs(&self, desc: &FalconUCodeDesc) -> Result<&[Bcrt30Rsa3kSignatu
         };
         // The signatures data follows the descriptor.
         let sigs_data_offset = self.falcon_ucode_offset + hdr_size;
-        let sigs_count = usize::from(desc.signature_count());
+        // V2 descriptors don't have signatures; unwrap_or(0) yields an empty slice.
+        let sigs_count = usize::from(desc.signature_count().unwrap_or(0));
         let sigs_size = sigs_count * core::mem::size_of::<Bcrt30Rsa3kSignature>();
 
         // Make sure the data is within bounds.

thanks,

Signature · 1 line
John Hubbard
Quoted 362 lines
(quoted diff) +80 -2
+
+/// Trait providing a common interface for accessing Falcon microcode descriptor fields.
+///
+/// This trait abstracts over the different descriptor versions ([`FalconUCodeDescV2`] and
+/// [`FalconUCodeDescV3`]), allowing code to work with firmware metadata without needing to
+/// know the specific descriptor version. Fields not present return zero.
+pub(crate) trait FalconUCodeDescriptor {
+    fn hdr(&self) -> u32;
+    fn imem_load_size(&self) -> u32;
+    fn interface_offset(&self) -> u32;
+    fn dmem_load_size(&self) -> u32;
+    fn pkc_data_offset(&self) -> u32;
+    fn engine_id_mask(&self) -> u16;
+    fn ucode_id(&self) -> u8;
+    fn signature_count(&self) -> u8;
+    fn signature_versions(&self) -> u16;
+
     /// Returns the size in bytes of the header.
-    pub(crate) fn size(&self) -> usize {
+    fn size(&self) -> usize {
+        let hdr = self.hdr();
+
         const HDR_SIZE_SHIFT: u32 = 16;
         const HDR_SIZE_MASK: u32 = 0xffff0000;
+        ((hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+    }
+}
+
+impl FalconUCodeDescriptor for FalconUCodeDescV2 {
+    fn hdr(&self) -> u32 {
+        self.hdr
+    }
+    fn imem_load_size(&self) -> u32 {
+        self.imem_load_size
+    }
+    fn interface_offset(&self) -> u32 {
+        self.interface_offset
+    }
+    fn dmem_load_size(&self) -> u32 {
+        self.dmem_load_size
+    }
+    fn pkc_data_offset(&self) -> u32 {
+        0
+    }
+    fn engine_id_mask(&self) -> u16 {
+        0
+    }
+    fn ucode_id(&self) -> u8 {
+        0
+    }
+    fn signature_count(&self) -> u8 {
+        0
+    }
+    fn signature_versions(&self) -> u16 {
+        0
+    }
+}
 
-        ((self.hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
+impl FalconUCodeDescriptor for FalconUCodeDescV3 {
+    fn hdr(&self) -> u32 {
+        self.hdr
+    }
+    fn imem_load_size(&self) -> u32 {
+        self.imem_load_size
+    }
+    fn interface_offset(&self) -> u32 {
+        self.interface_offset
+    }
+    fn dmem_load_size(&self) -> u32 {
+        self.dmem_load_size
+    }
+    fn pkc_data_offset(&self) -> u32 {
+        self.pkc_data_offset
+    }
+    fn engine_id_mask(&self) -> u16 {
+        self.engine_id_mask
+    }
+    fn ucode_id(&self) -> u8 {
+        self.ucode_id
+    }
+    fn signature_count(&self) -> u8 {
+        self.signature_count
+    }
+    fn signature_versions(&self) -> u16 {
+        self.signature_versions
     }
 }
drivers/gpu/nova-core/firmware/fwsec.rs +47 -25
index e4009faba6c5..1c1dcdacf5f5 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 +44 -31
index 7c26e4a2d61c..12256e5c8dc5 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,49 @@ 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 {
-            &*(self
-                .base
-                .data
-                .as_ptr()
-                .add(falcon_ucode_offset)
-                .cast::<FalconUCodeDescV3>())
-        })
+        let data = self
+            .base
+            .data
+            .get(falcon_ucode_offset..falcon_ucode_offset + hdr_size)
+            .ok_or(EINVAL)?;
+
+        match ver {
+            2 => {
+                let v2 = FalconUCodeDescV2::from_bytes(data).ok_or(EINVAL)?;
+                Ok(FalconUCodeDesc::V2(v2.clone()))
+            }
+            3 => {
+                let v3 = FalconUCodeDescV3::from_bytes(data).ok_or(EINVAL)?;
+                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 +1075,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.
author
#12 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

Quoted 6 lines · John Hubbard

It may have its own imperfections, but this is *approximately* what I'd prefer (below). I believe that it strikes the right balance between simplicity and Rust idiomatic code--and it's not any larger than what you started with.

And it won't cost you any time to implement, since this already works:

Do you realize that this is almost identical to my V1 of this patchset? We've come full circle.

#13 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

Quoted 10 lines · Timur Tabi
Quoted 6 lines · John Hubbard

It may have its own imperfections, but this is *approximately* what I'd prefer (below). I believe that it strikes the right balance between simplicity and Rust idiomatic code--and it's not any larger than what you started with.

And it won't cost you any time to implement, since this already works:

Do you realize that this is almost identical to my V1 of this patchset? We've come full circle.

I hope I didn't review that one in detail. This is really pretty funny. :)

thanks,

Signature · 1 line
John Hubbard
#14 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

Quoted 7 lines · John Hubbard
Quoted 6 lines · Timur Tabi
Quoted 5 lines · John Hubbard

It may have its own imperfections, but this is *approximately* what I'd prefer (below). I believe that it strikes the right balance between simplicity and Rust idiomatic code--and it's not any larger than what you started with.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

And it won't cost you any time to implement, since this already works:

Do you realize that this is almost identical to my V1 of this patchset?  We've come full circle.

I hope I didn't review that one in detail. This is really pretty funny. :)

But more seriously, we are all still calibrating ourselves about how far to go with various Rust abstractions, in various situations. Here, "we" means all of us involved in writing and reviewing the various Nova patchsets.

And we have different levels of experience with Rust, kernel, and GPU drivers. *And* that's changing as we continue to work together.

So I do expect this sort of situation to start converging over time. Meanwhile I want to apologize for you getting jerked around on reviews. It really will improve, I'm confident of that.

thanks,

Signature · 1 line
John Hubbard
#15 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

Quoted 11 lines · John Hubbard
Quoted 10 lines · John Hubbard
Quoted 9 lines · Timur Tabi
Quoted 6 lines · John Hubbard

It may have its own imperfections, but this is *approximately* what I'd prefer (below). I believe that it strikes the right balance between simplicity and Rust idiomatic code--and it's not any larger than what you started with.

And it won't cost you any time to implement, since this already works:

Do you realize that this is almost identical to my V1 of this patchset?  We've come full circle.

After chatting with Alex Courbot just now, we were thinking that we can debate these Rust abstraction improvements as future patches. Whichever way the debates go, I'm certainly OK with merging this working version asap, and making any changes in follow-up patches.

thanks,

Signature · 1 line
John Hubbard
#16 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

In reply to Timur Tabi

drivers/gpu/nova-core/firmware/fwsec.rs unsafe fn transmute_mut<T: Sized + FromBytes + AsBytes>( · line 247

Quoted 9 lines · Timur Tabi
 
     fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
drivers/gpu/nova-core/firmware/fwsec.rs +5 -2
-        // 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,

Since this subtraction's values comes from fw, it would be good to use checked_sub(). Otherwise, this could:

1. blow up if overflow checking is enabled.
2. len can underflow and be entirely plausible but incorrect, causing unpredictable failures.

I am fixing other similar existing issues in nova-core as well but since this patch is in flight, it'd be great to fix it in the next posting (it sounds like there will be a next posting).

thanks,

- Joel

author
#17 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

Quoted 10 lines · Joel Fernandes

Since this subtraction's values comes from fw, it would be good to use checked_sub(). Otherwise, this could:

1. blow up if overflow checking is enabled.
2. len can underflow and be entirely plausible but incorrect, causing unpredictable failures.

I am fixing other similar existing issues in nova-core as well but since this patch is in flight, it'd be great to fix it in the next posting (it sounds like there will be a next posting).

I'll make this fix for v6, but stuff like this is why I think the constructors for these objects should verify the bits of the images in fw, and return a Result<> if they are corrupted.

#18 lore raw

Re: [PATCH v5 09/11] gpu: nova-core: add FalconUCodeDescV2 support

Quoted 8 lines · Timur Tabi
Quoted 7 lines · Joel Fernandes

Since this subtraction's values comes from fw, it would be good to use checked_sub(). Otherwise, this could:

1. blow up if overflow checking is enabled.
2. len can underflow and be entirely plausible but incorrect, causing unpredictable failures.

Quoted 6 lines
Quoted 3 lines

I am fixing other similar existing issues in nova-core as well but since this patch is in flight, it'd be great to fix it in the next posting (it sounds like there will be a next posting).

I'll make this fix for v6, but stuff like this is why I think the constructors for these objects should verify the bits of the images in fw, and return a Result<> if they are corrupted.

Yeah, makes sense. AFAIK, what is agreed on is if we are 100% sure of the values (have checked them before), then we don't need to use checked arithmetic again at these sites as it is redundant.

Thanks.

author
#19 lore raw

PATCH v5 10/11 gpu: nova-core: align LibosMemoryRegionInitArgument size to page size

In reply to Timur Tabi

From: Alexandre Courbot <[email protected]>

On Turing and GA100 (i.e. the versions that use Libos v2), GSP-RM insists that the 'size' parameter of the LibosMemoryRegionInitArgument struct be aligned to 4KB. The logging buffers are already aligned to that size, so only the GSP_ARGUMENTS_CACHED struct needs to be adjusted. Make that adjustment by adding padding to the end of the struct.

---
 drivers/gpu/nova-core/firmware/fwsec.rs |  2 +-
 drivers/gpu/nova-core/gsp.rs            |  8 ++++----
 drivers/gpu/nova-core/gsp/fw.rs         | 14 +++++++++++++-
 3 files changed, 18 insertions(+), 6 deletions(-)
drivers/gpu/nova-core/firmware/fwsec.rs +1 -1
index 1c1dcdacf5f5..5d2323b07241 100644
@@ -433,7 +433,7 @@ pub(crate) fn new(
         };
 
         Ok(FwsecFirmware {
-            desc: desc,
+            desc,
             ucode: ucode_signed,
         })
     }
drivers/gpu/nova-core/gsp.rs +4 -4
index 766fd9905358..bcf6ce18a4a1 100644
@@ -27,7 +27,7 @@
 use crate::{
     gsp::cmdq::Cmdq,
     gsp::fw::{
-        GspArgumentsCached,
+        GspArgumentsAligned,
         LibosMemoryRegionInitArgument, //
     },
     num,
@@ -114,7 +114,7 @@ pub(crate) struct Gsp {
     /// Command queue.
     pub(crate) cmdq: Cmdq,
     /// RM arguments.
-    rmargs: CoherentAllocation<GspArgumentsCached>,
+    rmargs: CoherentAllocation<GspArgumentsAligned>,
 }
 
 impl Gsp {
@@ -133,7 +133,7 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                 logintr: LogBuffer::new(dev)?,
                 logrm: LogBuffer::new(dev)?,
                 cmdq: Cmdq::new(dev)?,
-                rmargs: CoherentAllocation::<GspArgumentsCached>::alloc_coherent(
+                rmargs: CoherentAllocation::<GspArgumentsAligned>::alloc_coherent(
                     dev,
                     1,
                     GFP_KERNEL | __GFP_ZERO,
@@ -149,7 +149,7 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                         libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
                     )?;
                     dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?;
-                    dma_write!(rmargs[0] = fw::GspArgumentsCached::new(cmdq))?;
+                    dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(cmdq))?;
                     dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
                 },
             }))
drivers/gpu/nova-core/gsp/fw.rs +13 -1
index abffd6beec65..15ca9c183ae1 100644
@@ -906,9 +906,21 @@ pub(crate) fn new(cmdq: &Cmdq) -> Self {
 // SAFETY: Padding is explicit and will not contain uninitialized data.
 unsafe impl AsBytes for GspArgumentsCached {}
 
+/// On Turing and GA100, the entries in the `LibosMemoryRegionInitArgument`
+/// must all be a multiple of GSP_PAGE_SIZE in size, so add padding to force it
+/// to that size.
+#[repr(C)]
+pub(crate) struct GspArgumentsAligned {
+    pub(crate) inner: GspArgumentsCached,
+    _padding: [u8; GSP_PAGE_SIZE - core::mem::size_of::<bindings::GSP_ARGUMENTS_CACHED>()],
+}
+
+// SAFETY: Padding is explicit and will not contain uninitialized data.
+unsafe impl AsBytes for GspArgumentsAligned {}
+
 // SAFETY: This struct only contains integer types for which all bit patterns
 // are valid.
-unsafe impl FromBytes for GspArgumentsCached {}
+unsafe impl FromBytes for GspArgumentsAligned {}
 
 /// Init arguments for the message queue.
 #[repr(transparent)]
Signature · 1 line
2.51.0
#20 lore raw

Re: [PATCH v5 10/11] gpu: nova-core: align LibosMemoryRegionInitArgument size to page size

Quoted 77 lines · Timur Tabi

From: Alexandre Courbot <[email protected]>

On Turing and GA100 (i.e. the versions that use Libos v2), GSP-RM insists that the 'size' parameter of the LibosMemoryRegionInitArgument struct be aligned to 4KB. The logging buffers are already aligned to that size, so only the GSP_ARGUMENTS_CACHED struct needs to be adjusted. Make that adjustment by adding padding to the end of the struct.

---
 drivers/gpu/nova-core/firmware/fwsec.rs |  2 +-
 drivers/gpu/nova-core/gsp.rs            |  8 ++++----
 drivers/gpu/nova-core/gsp/fw.rs         | 14 +++++++++++++-
 3 files changed, 18 insertions(+), 6 deletions(-)
drivers/gpu/nova-core/firmware/fwsec.rs +1 -1
index 1c1dcdacf5f5..5d2323b07241 100644
@@ -433,7 +433,7 @@ pub(crate) fn new(
         };
 
         Ok(FwsecFirmware {
-            desc: desc,
+            desc,
             ucode: ucode_signed,
         })
     }
drivers/gpu/nova-core/gsp.rs +4 -4
index 766fd9905358..bcf6ce18a4a1 100644
@@ -27,7 +27,7 @@
 use crate::{
     gsp::cmdq::Cmdq,
     gsp::fw::{
-        GspArgumentsCached,
+        GspArgumentsAligned,
         LibosMemoryRegionInitArgument, //
     },
     num,
@@ -114,7 +114,7 @@ pub(crate) struct Gsp {
     /// Command queue.
     pub(crate) cmdq: Cmdq,
     /// RM arguments.
-    rmargs: CoherentAllocation<GspArgumentsCached>,
+    rmargs: CoherentAllocation<GspArgumentsAligned>,
 }
 
 impl Gsp {
@@ -133,7 +133,7 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                 logintr: LogBuffer::new(dev)?,
                 logrm: LogBuffer::new(dev)?,
                 cmdq: Cmdq::new(dev)?,
-                rmargs: CoherentAllocation::<GspArgumentsCached>::alloc_coherent(
+                rmargs: CoherentAllocation::<GspArgumentsAligned>::alloc_coherent(
                     dev,
                     1,
                     GFP_KERNEL | __GFP_ZERO,
@@ -149,7 +149,7 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                         libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
                     )?;
                     dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?;
-                    dma_write!(rmargs[0] = fw::GspArgumentsCached::new(cmdq))?;
+                    dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(cmdq))?;
                     dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
                 },
             }))
drivers/gpu/nova-core/gsp/fw.rs +0 -0
index abffd6beec65..15ca9c183ae1 100644
@@ -906,9 +906,21 @@ pub(crate) fn new(cmdq: &Cmdq) -> Self {
 // SAFETY: Padding is explicit and will not contain uninitialized data.
 unsafe impl AsBytes for GspArgumentsCached {}
 

drivers/gpu/nova-core/gsp/fw.rs pub(crate) fn new(cmdq: &Cmdq) -> Self { · line 918

Quoted 10 lines
drivers/gpu/nova-core/gsp/fw.rs +10 -0
+/// On Turing and GA100, the entries in the `LibosMemoryRegionInitArgument`
+/// must all be a multiple of GSP_PAGE_SIZE in size, so add padding to force it
+/// to that size.
+#[repr(C)]
+pub(crate) struct GspArgumentsAligned {
+    pub(crate) inner: GspArgumentsCached,
+    _padding: [u8; GSP_PAGE_SIZE - core::mem::size_of::<bindings::GSP_ARGUMENTS_CACHED>()],
+}
+
+// SAFETY: Padding is explicit and will not contain uninitialized data.

Sort of a pre-existing problem, but "Padding is explicit" seems meaningless here. And if we're going to write anything here, maybe we should say *why* we know that the padding can't or won't contain uninitialized data?

Otherwise it all looks good.

drivers/gpu/nova-core/gsp/fw.rs pub(crate) fn new(cmdq: &Cmdq) -> Self { · line 926

Quoted 9 lines
drivers/gpu/nova-core/gsp/fw.rs +3 -1
+unsafe impl AsBytes for GspArgumentsAligned {}
+
 // SAFETY: This struct only contains integer types for which all bit patterns
 // are valid.
-unsafe impl FromBytes for GspArgumentsCached {}
+unsafe impl FromBytes for GspArgumentsAligned {}
 
 /// Init arguments for the message queue.
 #[repr(transparent)]

thanks,

Signature · 1 line
John Hubbard
author
#21 lore raw

PATCH v5 11/11 gpu: nova-core: add PIO support for loading firmware images

In reply to Timur Tabi

Turing and GA100 use programmed I/O (PIO) instead of DMA to upload firmware images into Falcon memory.

A new firmware called the Generic Bootloader (as opposed to the GSP Bootloader) is used to upload FWSEC.

---
 drivers/gpu/nova-core/falcon.rs           | 185 +++++++++++++++++++++-
 drivers/gpu/nova-core/falcon/hal.rs       |   9 +-
 drivers/gpu/nova-core/falcon/hal/ga102.rs |   5 +-
 drivers/gpu/nova-core/falcon/hal/tu102.rs |   7 +-
 drivers/gpu/nova-core/firmware/fwsec.rs   | 124 ++++++++++++++-
 drivers/gpu/nova-core/gsp/boot.rs         |   6 +-
 drivers/gpu/nova-core/regs.rs             |  30 ++++
 7 files changed, 350 insertions(+), 16 deletions(-)
drivers/gpu/nova-core/falcon.rs +180 -5
index 744f513da9cd..be144bd139fb 100644
@@ -15,11 +15,17 @@
     time::{
         Delta, //
     },
+    transmute::AsBytes, //
 };
 
 use crate::{
     dma::DmaObject,
     driver::Bar0,
+    falcon::hal::LoadMethod,
+    firmware::fwsec::{
+        BootloaderDmemDescV2,
+        GenericBootloader, //
+    },
     gpu::Chipset,
     num::{
         FromSafeCast,
@@ -406,6 +412,169 @@ pub(crate) fn reset(&self, bar: &Bar0) -> Result {
         Ok(())
     }
 
+    /// Write a byte array to Falcon memory using programmed I/O (PIO).
+    ///
+    /// Writes `img` to the specified `target_mem` (IMEM or DMEM) starting at `mem_base`.
+    /// For IMEM writes, tags are set for each 256-byte block starting from `tag`.
+    ///
+    /// Returns `EINVAL` if `img.len()` is not a multiple of 4.
+    fn pio_wr_bytes(
+        &self,
+        bar: &Bar0,
+        img: &[u8],
+        mem_base: u16,
+        target_mem: FalconMem,
+        port: u8,
+        tag: u16,
+    ) -> Result {
+        // Rejecting misaligned images here allows us to avoid checking
+        // inside the loops.
+        if img.len() % 4 != 0 {
+            return Err(EINVAL);
+        }
+
+        let port = usize::from(port);
+
+        match target_mem {
+            FalconMem::ImemSecure | FalconMem::ImemNonSecure => {
+                regs::NV_PFALCON_FALCON_IMEMC::default()
+                    .set_secure(target_mem == FalconMem::ImemSecure)
+                    .set_aincw(true)
+                    .set_offs(mem_base)
+                    .write(bar, &E::ID, port);
+
+                let mut tag = tag;
+                for block in img.chunks(256) {
+                    regs::NV_PFALCON_FALCON_IMEMT::default()
+                        .set_tag(tag)
+                        .write(bar, &E::ID, port);
+                    for word in block.chunks_exact(4) {
+                        let w = [word[0], word[1], word[2], word[3]];
+                        regs::NV_PFALCON_FALCON_IMEMD::default()
+                            .set_data(u32::from_le_bytes(w))
+                            .write(bar, &E::ID, port);
+                    }
+                    tag += 1;
+                }
+            }
+            FalconMem::Dmem => {
+                regs::NV_PFALCON_FALCON_DMEMC::default()
+                    .set_aincw(true)
+                    .set_offs(mem_base)
+                    .write(bar, &E::ID, port);
+
+                for block in img.chunks(256) {
+                    for word in block.chunks_exact(4) {
+                        let w = [word[0], word[1], word[2], word[3]];
+                        regs::NV_PFALCON_FALCON_DMEMD::default()
+                            .set_data(u32::from_le_bytes(w))
+                            .write(bar, &E::ID, port);
+                    }
+                }
+            }
+        }
+
+        Ok(())
+    }
+
+    fn pio_wr<F: FalconFirmware<Target = E>>(
+        &self,
+        bar: &Bar0,
+        fw: &F,
+        target_mem: FalconMem,
+        load_offsets: &FalconLoadTarget,
+        port: u8,
+        tag: u16,
+    ) -> Result {
+        let start = usize::from_safe_cast(load_offsets.src_start);
+        let len = usize::from_safe_cast(load_offsets.len);
+        let mem_base = u16::try_from(load_offsets.dst_start)?;
+
+        // SAFETY: we are the only user of the firmware image at this stage
+        let data = unsafe { fw.as_slice(start, len).map_err(|_| EINVAL)? };
+
+        self.pio_wr_bytes(bar, data, mem_base, target_mem, port, tag)
+    }
+
+    /// Perform a PIO copy into `IMEM` and `DMEM` of `fw`, and prepare the falcon to run it.
+    pub(crate) fn pio_load<F: FalconFirmware<Target = E>>(
+        &self,
+        bar: &Bar0,
+        fw: &F,
+        gbl: Option<&GenericBootloader>,
+    ) -> Result {
+        let imem_sec = fw.imem_sec_load_params();
+        let imem_ns = fw.imem_ns_load_params().ok_or(EINVAL)?;
+        let dmem = fw.dmem_load_params();
+
+        regs::NV_PFALCON_FBIF_CTL::read(bar, &E::ID)
+            .set_allow_phys_no_ctx(true)
+            .write(bar, &E::ID);
+
+        regs::NV_PFALCON_FALCON_DMACTL::default().write(bar, &E::ID);
+
+        // If the Generic Bootloader was passed, then use it to boot FRTS
+        if let Some(gbl) = gbl {
+            let dst_start = u16::try_from(0x10000 - gbl.desc.code_size)?;
+            let data = &gbl.ucode[..usize::from_safe_cast(gbl.desc.code_size)];
+            let tag = u16::try_from(gbl.desc.start_tag)?;
+
+            self.pio_wr_bytes(bar, data, dst_start, FalconMem::ImemNonSecure, 0, tag)?;
+
+            // This structure tells the generic bootloader where to find the FWSEC
+            // image.
+            let dmem_desc = BootloaderDmemDescV2 {
+                reserved: [0; 4],
+                signature: [0; 4],
+                ctx_dma: 4, // FALCON_DMAIDX_PHYS_SYS_NCOH
+                code_dma_base: fw.dma_handle(),
+                non_sec_code_off: imem_ns.dst_start,
+                non_sec_code_size: imem_ns.len,
+                sec_code_off: imem_sec.dst_start,
+                sec_code_size: imem_sec.len,
+                code_entry_point: 0,
+                data_dma_base: fw.dma_handle() + u64::from(dmem.src_start),
+                data_size: dmem.len,
+                argc: 0,
+                argv: 0,
+            };
+
+            regs::NV_PFALCON_FBIF_TRANSCFG::update(bar, &E::ID, 4, |v| {
+                v.set_target(FalconFbifTarget::CoherentSysmem)
+                    .set_mem_type(FalconFbifMemType::Physical)
+            });
+
+            self.pio_wr_bytes(bar, dmem_desc.as_bytes(), 0, FalconMem::Dmem, 0, 0)?;
+        } else {
+            self.pio_wr(
+                bar,
+                fw,
+                FalconMem::ImemNonSecure,
+                &imem_ns,
+                0,
+                u16::try_from(imem_ns.dst_start >> 8)?,
+            )?;
+            self.pio_wr(
+                bar,
+                fw,
+                FalconMem::ImemSecure,
+                &imem_sec,
+                0,
+                u16::try_from(imem_sec.dst_start >> 8)?,
+            )?;
+            self.pio_wr(bar, fw, FalconMem::Dmem, &dmem, 0, 0)?;
+        }
+
+        self.hal.program_brom(self, bar, &fw.brom_params())?;
+
+        // Set `BootVec` to start of non-secure code.
+        regs::NV_PFALCON_FALCON_BOOTVEC::default()
+            .set_value(fw.boot_addr())
+            .write(bar, &E::ID);
+
+        Ok(())
+    }
+
     /// Perform a DMA write according to `load_offsets` from `dma_handle` into the falcon's
     /// `target_mem`.
     ///
@@ -629,11 +798,17 @@ pub(crate) fn is_riscv_active(&self, bar: &Bar0) -> bool {
         self.hal.is_riscv_active(bar)
     }
 
-    /// Check if this Falcon supports DMA for loading firmware.
-    ///
-    /// Returns `true` if DMA is supported, `false` if PIO must be used instead.
-    pub(crate) fn supports_dma(&self) -> bool {
-        self.hal.supports_dma()
+    // Load a firmware image into Falcon memory
+    pub(crate) fn load<F: FalconFirmware<Target = E>>(
+        &self,
+        bar: &Bar0,
+        fw: &F,
+        gbl: Option<&GenericBootloader>,
+    ) -> Result {
+        match self.hal.load_method() {
+            LoadMethod::Pio => self.pio_load(bar, fw, gbl),
+            LoadMethod::Dma => self.dma_load(bar, fw),
+        }
     }
 
     /// Write the application version to the OS register.
drivers/gpu/nova-core/falcon/hal.rs +7 -2
index 49501aa6ff7f..3a882b7d8aa8 100644
@@ -15,6 +15,11 @@
 mod ga102;
 mod tu102;
 
+pub(crate) enum LoadMethod {
+    Pio,
+    Dma,
+}
+
 /// Hardware Abstraction Layer for Falcon cores.
 ///
 /// Implements chipset-specific low-level operations. The trait is generic against [`FalconEngine`]
@@ -49,8 +54,8 @@ fn signature_reg_fuse_version(
     /// Reset the falcon engine.
     fn reset_eng(&self, bar: &Bar0) -> Result;
 
-    /// Returns true of this Falcon supports DMA transfer from system memory to Falcon memory
-    fn supports_dma(&self) -> bool;
+    /// returns the method needed to load data into Falcon memory
+    fn load_method(&self) -> LoadMethod;
 }
 
 /// Returns a boxed falcon HAL adequate for `chipset`.
drivers/gpu/nova-core/falcon/hal/ga102.rs +3 -2
index b2857293e653..00a78510b7f1 100644
@@ -17,6 +17,7 @@
         FalconBromParams,
         FalconEngine,
         FalconModSelAlgo,
+        hal::LoadMethod,
         PeregrineCoreSelect, //
     },
     regs,
@@ -159,7 +160,7 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
         Ok(())
     }
 
-    fn supports_dma(&self) -> bool {
-        true
+    fn load_method(&self) -> LoadMethod {
+        LoadMethod::Dma
     }
 }
drivers/gpu/nova-core/falcon/hal/tu102.rs +4 -3
index 28dc03db8240..5fe578f6667d 100644
@@ -14,7 +14,8 @@
     falcon::{
         Falcon,
         FalconBromParams,
-        FalconEngine, //
+        FalconEngine,
+        hal::LoadMethod, //
     },
     regs, //
 };
@@ -77,7 +78,7 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
         Ok(())
     }
 
-    fn supports_dma(&self) -> bool {
-        false
+    fn load_method(&self) -> LoadMethod {
+        LoadMethod::Pio
     }
 }
drivers/gpu/nova-core/firmware/fwsec.rs +122 -2

175 lines not on this page · raw message

drivers/gpu/nova-core/gsp/boot.rs +4 -2
index a53d80620468..0127138644b9 100644
@@ -48,6 +48,7 @@ impl super::Gsp {
     /// created the WPR2 region.
     fn run_fwsec_frts(
         dev: &device::Device<device::Bound>,
+        chipset: Chipset,
         falcon: &Falcon<Gsp>,
         bar: &Bar0,
         bios: &Vbios,
@@ -65,6 +66,7 @@ fn run_fwsec_frts(
 
         let fwsec_frts = FwsecFirmware::new(
             dev,
+            chipset,
             falcon,
             bar,
             bios,
@@ -144,7 +146,7 @@ pub(crate) fn boot(
         let fb_layout = FbLayout::new(chipset, bar, &gsp_fw)?;
         dev_dbg!(dev, "{:#x?}\n", fb_layout);
 
-        Self::run_fwsec_frts(dev, gsp_falcon, bar, &bios, &fb_layout)?;
+        Self::run_fwsec_frts(dev, chipset, gsp_falcon, bar, &bios, &fb_layout)?;
 
         let booter_loader = BooterFirmware::new(
             dev,
@@ -183,7 +185,7 @@ pub(crate) fn boot(
         );
 
         sec2_falcon.reset(bar)?;
-        sec2_falcon.dma_load(bar, &booter_loader)?;
+        sec2_falcon.load(bar, &booter_loader, None)?;
         let wpr_handle = wpr_meta.dma_handle();
         let (mbox0, mbox1) = sec2_falcon.boot(
             bar,
drivers/gpu/nova-core/regs.rs +30 -0
index cd7b7aa6fc2a..f0129a5f7a7e 100644
@@ -359,6 +359,36 @@ pub(crate) fn with_falcon_mem(self, mem: FalconMem) -> Self {
     1:1     startcpu as bool;
 });
 
+// IMEM access control register. Up to 4 ports are available for IMEM access.
+register!(NV_PFALCON_FALCON_IMEMC @ PFalconBase[0x00000180[4; 16]] {
+    15:0      offs as u16, "IMEM block and word offset";
+    24:24     aincw as bool, "Auto-increment on write";
+    28:28     secure as bool, "Access secure IMEM";
+});
+
+// IMEM data register. Reading/writing this register accesses IMEM at the address
+// specified by the corresponding IMEMC register.
+register!(NV_PFALCON_FALCON_IMEMD @ PFalconBase[0x00000184[4; 16]] {
+    31:0      data as u32;
+});
+
+// IMEM tag register. Used to set the tag for the current IMEM block.
+register!(NV_PFALCON_FALCON_IMEMT @ PFalconBase[0x00000188[4; 16]] {
+    15:0      tag as u16;
+});
+
+// DMEM access control register. Up to 8 ports are available for DMEM access.
+register!(NV_PFALCON_FALCON_DMEMC @ PFalconBase[0x000001c0[8; 8]] {
+    15:0      offs as u16, "DMEM block and word offset";
+    24:24     aincw as bool, "Auto-increment on write";
+});
+
+// DMEM data register. Reading/writing this register accesses DMEM at the address
+// specified by the corresponding DMEMC register.
+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] {
Signature · 1 line
2.51.0
#22 lore raw

Re: [PATCH v5 11/11] gpu: nova-core: add PIO support for loading firmware images

kernel test robot noticed the following build errors:

Hi Timur,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7acc70476f14661149774ab88d3fe23d83ba4249]

url:    https://github.com/intel-lab-lkp/linux/commits/Timur-Tabi/gpu-nova-core-rename-Imem-to-ImemSecure/20260103-130228
base:   7acc70476f14661149774ab88d3fe23d83ba4249
patch link:    https://lore.kernel.org/r/20260103045934.64521-12-ttabi%40nvidia.com
patch subject: [PATCH v5 11/11] gpu: nova-core: add PIO support for loading firmware images
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20260103/[email protected]/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260103/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

PATH=/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
INFO PATH=/opt/cross/rustc-1.88.0-bindgen-0.72.1/cargo/bin:/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
/usr/bin/timeout -k 100 12h /usr/bin/make KCFLAGS= -fno-crash-diagnostics -Wno-error=return-type -Wreturn-type -funsigned-char -Wundef W=1 --keep-going LLVM=1 -j32 -C source O=/kbuild/obj/consumer/x86_64-rhel-9.4-rust ARCH=x86_64 SHELL=/bin/bash rustfmtcheck
make: Entering directory '/kbuild/src/consumer'
make[1]: Entering directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'

>> Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:13:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod,
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:19:
            FalconModSelAlgo,
   -        hal::LoadMethod,
            PeregrineCoreSelect, //
        },
        regs,
>> Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:12:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod, //
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:18:
   -        hal::LoadMethod, //
        },
        regs, //
    };

Diff in drivers/gpu/nova-core/falcon.rs:674:

        /// 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() {
>> Diff in drivers/gpu/nova-core/firmware/fwsec.rs:516:
            // console error message on other platforms, only try to load it if it's
            // supposed to be there.
            let gbl_fw = if chipset < Chipset::GA102 {
   -            Some(super::request_firmware(dev, chipset, "gen_bootloader", FIRMWARE_VERSION)?)
   +            Some(super::request_firmware(
   +                dev,
   +                chipset,
   +                "gen_bootloader",
   +                FIRMWARE_VERSION,
   +            )?)
            } else {
                None
            };
>> Diff in drivers/gpu/nova-core/firmware/fwsec.rs:516:
            // console error message on other platforms, only try to load it if it's
            // supposed to be there.
            let gbl_fw = if chipset < Chipset::GA102 {
   -            Some(super::request_firmware(dev, chipset, "gen_bootloader", FIRMWARE_VERSION)?)
   +            Some(super::request_firmware(
   +                dev,
   +                chipset,
   +                "gen_bootloader",
   +                FIRMWARE_VERSION,
   +            )?)
            } else {
                None
            };
>> Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:13:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod,
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:19:
            FalconModSelAlgo,
   -        hal::LoadMethod,
            PeregrineCoreSelect, //
        },
        regs,
>> Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:12:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod, //
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:18:
   -        hal::LoadMethod, //
        },
        regs, //
    };

Diff in drivers/gpu/nova-core/falcon.rs:674:

        /// 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() {
>> Diff in drivers/gpu/nova-core/firmware/fwsec.rs:516:
            // console error message on other platforms, only try to load it if it's
            // supposed to be there.
            let gbl_fw = if chipset < Chipset::GA102 {
   -            Some(super::request_firmware(dev, chipset, "gen_bootloader", FIRMWARE_VERSION)?)
   +            Some(super::request_firmware(
   +                dev,
   +                chipset,
   +                "gen_bootloader",
   +                FIRMWARE_VERSION,
   +            )?)
            } else {
                None
            };
>> Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:13:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod,
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:19:
            FalconModSelAlgo,
   -        hal::LoadMethod,
            PeregrineCoreSelect, //
        },
        regs,
>> Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:12:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod, //
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:18:
   -        hal::LoadMethod, //
        },
        regs, //
    };
>> Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:13:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod,
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/ga102.rs:19:
            FalconModSelAlgo,
   -        hal::LoadMethod,
            PeregrineCoreSelect, //
        },
        regs,
>> Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:12:
    use crate::{
        driver::Bar0,
        falcon::{
   +        hal::LoadMethod, //
            Falcon,
            FalconBromParams,
            FalconEngine,
   Diff in drivers/gpu/nova-core/falcon/hal/tu102.rs:18:
   -        hal::LoadMethod, //
        },
        regs, //
    };

make[2]: *** [Makefile:1871: rustfmt] Error 123
make[2]: Target 'rustfmtcheck' not remade because of errors. make[1]: Leaving directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust' make[1]: *** [Makefile:248: __sub-make] Error 2
make[1]: Target 'rustfmtcheck' not remade because of errors.
make: *** [Makefile:248: __sub-make] Error 2
make: Target 'rustfmtcheck' not remade because of errors.
make: Leaving directory '/kbuild/src/consumer'

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
author
#23 lore raw
In reply to Timur Tabi
Quoted 2 lines · Timur Tabi

This patch set adds basic support for pre-booting GSP-RM on Turing.

I found a few minor issues (e.g. rustfmt) that will be fixed in v6. I'll wait until I get feedback on v5 before posting it.