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

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

On Thu Jan 15, 2026 at 4:29 AM JST, Timur Tabi wrote:
> The FRTS firmware in Turing and GA100 VBIOS has an older header
> format (v2 instead of v3).  To support both v2 and v3 at runtime,
> add the FalconUCodeDescV2 struct, and update code that references
> the FalconUCodeDescV3 directly with a FalconUCodeDesc enum that
> encapsulates both.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  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(-)
>
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 2d2008b33fb4..9f0ad02fbe22 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -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
>      }
>  }
>  
> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
> index e4009faba6c5..c4fff8b86640 100644
> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
> @@ -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.checked_sub(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(),
>          }
>      }

It looks like John and I have a different opinion on which approach is
the best to handle the v2/v3 headers (a match in each method vs a new
trait and virtual method calls).

TBH I don't really mind which approach we adopt, but I would like us to
stay consistent through the code. This patch adds the trait and
implements `Deref`, but then the chunk above still performs matches.

I did suggest that we add more methods to the trait in v4, but to be
clear here is the doing what I suggested. However if you prefer to drop
the trait and perform a match in every method of `FalconUCodeDesc`,
that's also acceptable to me - in this case the matches you did above
would be consistent with the general approach and can be kept as-is.

diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 9f0ad02fbe22..68779540aa28 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -16,7 +16,10 @@
 
 use crate::{
     dma::DmaObject,
-    falcon::FalconFirmware,
+    falcon::{
+        FalconFirmware,
+        FalconLoadTarget, //
+    },
     gpu,
     num::{
         FromSafeCast,
@@ -167,6 +170,10 @@ fn size(&self) -> usize {
         const HDR_SIZE_MASK: u32 = 0xffff0000;
         ((hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
     }
+
+    fn imem_sec_load_params(&self) -> FalconLoadTarget;
+    fn imem_ns_load_params(&self) -> Option<FalconLoadTarget>;
+    fn dmem_load_params(&self) -> FalconLoadTarget;
 }
 
 impl FalconUCodeDescriptor for FalconUCodeDescV2 {
@@ -197,6 +204,30 @@ fn signature_count(&self) -> u8 {
     fn signature_versions(&self) -> u16 {
         0
     }
+
+    fn imem_sec_load_params(&self) -> FalconLoadTarget {
+        FalconLoadTarget {
+            src_start: 0,
+            dst_start: self.imem_sec_base,
+            len: self.imem_sec_size,
+        }
+    }
+
+    fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
+        Some(FalconLoadTarget {
+            src_start: 0,
+            dst_start: self.imem_phys_base,
+            len: self.imem_load_size.checked_sub(self.imem_sec_size)?,
+        })
+    }
+
+    fn dmem_load_params(&self) -> FalconLoadTarget {
+        FalconLoadTarget {
+            src_start: self.dmem_offset,
+            dst_start: self.dmem_phys_base,
+            len: self.dmem_load_size,
+        }
+    }
 }
 
 impl FalconUCodeDescriptor for FalconUCodeDescV3 {
@@ -227,6 +258,27 @@ fn signature_count(&self) -> u8 {
     fn signature_versions(&self) -> u16 {
         self.signature_versions
     }
+
+    fn imem_sec_load_params(&self) -> FalconLoadTarget {
+        FalconLoadTarget {
+            src_start: 0,
+            dst_start: self.imem_phys_base,
+            len: self.imem_load_size,
+        }
+    }
+
+    fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
+        // Not used on V3 platforms
+        None
+    }
+
+    fn dmem_load_params(&self) -> FalconLoadTarget {
+        FalconLoadTarget {
+            src_start: self.imem_load_size,
+            dst_start: self.dmem_phys_base,
+            len: self.dmem_load_size,
+        }
+    }
 }
 
 /// Trait implemented by types defining the signed state of a firmware.
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
index a5678db8f78c..4efa4711fce1 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -292,45 +292,15 @@ pub(crate) struct FwsecFirmware {
 
 impl FalconLoadParams for FwsecFirmware {
     fn imem_sec_load_params(&self) -> FalconLoadTarget {
-        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,
-            },
-        }
+        self.desc.imem_sec_load_params()
     }
 
     fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
-        match &self.desc {
-            FalconUCodeDesc::V2(v2) => Some(FalconLoadTarget {
-                src_start: 0,
-                dst_start: v2.imem_phys_base,
-                len: v2.imem_load_size.checked_sub(v2.imem_sec_size)?,
-            }),
-            // Not used on V3 platforms
-            FalconUCodeDesc::V3(_v3) => None,
-        }
+        self.desc.imem_ns_load_params()
     }
 
     fn dmem_load_params(&self) -> FalconLoadTarget {
-        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,
-            },
-        }
+        self.desc.dmem_load_params()
     }
 
     fn brom_params(&self) -> FalconBromParams {