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

[Joel Fernandes <joelagnelf@nvidia.com>]

On Fri, Nov 14, 2025 at 05:30:42PM -0600, 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       | 108 +++++++++++++++++++++++-
>  drivers/gpu/nova-core/firmware/fwsec.rs |  72 ++++++++++------
>  drivers/gpu/nova-core/vbios.rs          |  74 ++++++++++------
>  3 files changed, 202 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 2d2008b33fb4..5ca5bf1fb610 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -43,6 +43,43 @@ fn request_firmware(
>          .and_then(|path| firmware::Firmware::request(&path, dev))
>  }
>  
> +/// Structure used to describe some firmwares, notably FWSEC-FRTS.
> +#[repr(C)]
> +#[derive(Debug, Clone)]
> +pub(crate) struct FalconUCodeDescV2 {
> +    /// Header defined by 'NV_BIT_FALCON_UCODE_DESC_HEADER_VDESC*' in OpenRM.
> +    hdr: u32,
> +    /// Stored size of the ucode after the header, compressed or uncompressed
> +    stored_size: u32,
> +    /// Uncompressed size of the ucode.  If store_size == uncompressed_size, then the ucode
> +    /// is not compressed.
> +    pub(crate) uncompressed_size: u32,
> +    /// Code entry point
> +    pub(crate) virtual_entry: u32,
> +    /// Offset after the code segment at which the Application Interface Table headers are located.
> +    pub(crate) interface_offset: u32,
> +    /// Base address at which to load the code segment into 'IMEM'.
> +    pub(crate) imem_phys_base: u32,
> +    /// Size in bytes of the code to copy into 'IMEM'.
> +    pub(crate) imem_load_size: u32,
> +    /// Virtual 'IMEM' address (i.e. 'tag') at which the code should start.
> +    pub(crate) imem_virt_base: u32,
> +    /// Virtual address of secure IMEM segment.
> +    pub(crate) imem_sec_base: u32,
> +    /// Size of secure IMEM segment.
> +    pub(crate) imem_sec_size: u32,
> +    /// Offset into stored (uncompressed) image at which DMEM begins.
> +    pub(crate) dmem_offset: u32,
> +    /// Base address at which to load the data segment into 'DMEM'.
> +    pub(crate) dmem_phys_base: u32,
> +    /// Size in bytes of the data to copy into 'DMEM'.
> +    pub(crate) dmem_load_size: u32,
> +    /// "Alternate" Size of data to load into IMEM.
> +    pub(crate) alt_imem_load_size: u32,
> +    /// "Alternate" Size of data to load into DMEM.
> +    pub(crate) alt_dmem_load_size: u32,
> +}
> +
>  /// Structure used to describe some firmwares, notably FWSEC-FRTS.
>  #[repr(C)]
>  #[derive(Debug, Clone)]
> @@ -76,13 +113,80 @@ pub(crate) struct FalconUCodeDescV3 {
>      _reserved: u16,
>  }
>  
> -impl FalconUCodeDescV3 {
> +#[derive(Debug, Clone)]
> +pub(crate) enum FalconUCodeDesc {
> +    V2(FalconUCodeDescV2),
> +    V3(FalconUCodeDescV3),
> +}
> +
> +impl FalconUCodeDesc {
>      /// Returns the size in bytes of the header.
>      pub(crate) fn size(&self) -> usize {
> +        let hdr = match self {
> +            FalconUCodeDesc::V2(v2) => v2.hdr,
> +            FalconUCodeDesc::V3(v3) => v3.hdr,
> +        };
> +
>          const HDR_SIZE_SHIFT: u32 = 16;
>          const HDR_SIZE_MASK: u32 = 0xffff0000;
> +        ((hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
> +    }
> +
> +    pub(crate) fn imem_load_size(&self) -> u32 {
> +        match self {
> +            FalconUCodeDesc::V2(v2) => v2.imem_load_size,
> +            FalconUCodeDesc::V3(v3) => v3.imem_load_size,
> +        }
> +    }


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

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

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

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

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

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

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

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

// Usage example - no more match statements needed!
impl FalconLoadParams for FwsecFirmware {
    fn dmem_load_params(&self) -> FalconLoadTarget {
        FalconLoadTarget {
            src_start: 0,
            dst_start: 0,
            len: self.desc.dmem_load_size(),
        }
    }
}


thanks,

 - Joel


> +
> +    pub(crate) fn interface_offset(&self) -> u32 {
> +        match self {
> +            FalconUCodeDesc::V2(v2) => v2.interface_offset,
> +            FalconUCodeDesc::V3(v3) => v3.interface_offset,
> +        }
> +    }
> +
> +
> +    pub(crate) fn dmem_load_size(&self) -> u32 {
> +        match self {
> +            FalconUCodeDesc::V2(v2) => v2.dmem_load_size,
> +            FalconUCodeDesc::V3(v3) => v3.dmem_load_size,
> +        }
> +    }
> +
> +    pub(crate) fn pkc_data_offset(&self) -> u32 {
> +        match self {
> +            FalconUCodeDesc::V2(_v2) => 0,
> +            FalconUCodeDesc::V3(v3) => v3.pkc_data_offset,
> +        }
> +    }
>  
> -        ((self.hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
> +    pub(crate) fn engine_id_mask(&self) -> u16 {
> +        match self {
> +            FalconUCodeDesc::V2(_v2) => 0,
> +            FalconUCodeDesc::V3(v3) => v3.engine_id_mask,
> +        }
> +    }
> +
> +    pub(crate) fn ucode_id(&self) -> u8 {
> +        match self {
> +            FalconUCodeDesc::V2(_v2) => 0,
> +            FalconUCodeDesc::V3(v3) => v3.ucode_id,
> +        }
> +    }
> +
> +    pub(crate) fn signature_count(&self) -> u8 {
> +        match self {
> +            FalconUCodeDesc::V2(_v2) => 0,
> +            FalconUCodeDesc::V3(v3) => v3.signature_count,
> +        }
> +    }
> +
> +    pub(crate) fn signature_versions(&self) -> u16 {
> +        match self {
> +            FalconUCodeDesc::V2(_v2) => 0,
> +            FalconUCodeDesc::V3(v3) => v3.signature_versions,
> +        }
>      }
>  }
>  
> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
> index e4009faba6c5..36ff8ed51c23 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 - 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,
>          })
>      }
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index abf423560ff4..860d6fb3f516 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -19,6 +19,8 @@
>      driver::Bar0,
>      firmware::{
>          fwsec::Bcrt30Rsa3kSignature,
> +        FalconUCodeDesc,
> +        FalconUCodeDescV2,
>          FalconUCodeDescV3, //
>      },
>      num::FromSafeCast,
> @@ -1004,19 +1006,10 @@ fn build(self) -> Result<FwSecBiosImage> {
>  
>  impl FwSecBiosImage {
>      /// Get the FwSec header ([`FalconUCodeDescV3`]).
> -    pub(crate) fn header(&self) -> Result<&FalconUCodeDescV3> {
> +    pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
>          // Get the falcon ucode offset that was found in setup_falcon_data.
>          let falcon_ucode_offset = self.falcon_ucode_offset;
>  
> -        // Make sure the offset is within the data bounds.
> -        if falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>() > self.base.data.len() {
> -            dev_err!(
> -                self.base.dev,
> -                "fwsec-frts header not contained within BIOS bounds\n"
> -            );
> -            return Err(ERANGE);
> -        }
> -
>          // Read the first 4 bytes to get the version.
>          let hdr_bytes: [u8; 4] = self.base.data[falcon_ucode_offset..falcon_ucode_offset + 4]
>              .try_into()
> @@ -1024,33 +1017,60 @@ pub(crate) fn header(&self) -> Result<&FalconUCodeDescV3> {
>          let hdr = u32::from_le_bytes(hdr_bytes);
>          let ver = (hdr & 0xff00) >> 8;
>  
> -        if ver != 3 {
> -            dev_err!(self.base.dev, "invalid fwsec firmware version: {:?}\n", ver);
> -            return Err(EINVAL);
> +        let hdr_size = match ver {
> +            2 => core::mem::size_of::<FalconUCodeDescV2>(),
> +            3 => core::mem::size_of::<FalconUCodeDescV3>(),
> +            _ => {
> +                dev_err!(self.base.dev, "invalid fwsec firmware version: {:?}\n", ver);
> +                return Err(EINVAL);
> +            }
> +        };
> +        // Make sure the offset is within the data bounds
> +        if falcon_ucode_offset + hdr_size > self.base.data.len() {
> +            dev_err!(
> +                self.base.dev,
> +                "fwsec-frts header not contained within BIOS bounds\n"
> +            );
> +            return Err(ERANGE);
>          }
>  
> -        // Return a reference to the FalconUCodeDescV3 structure.
> -        //
> -        // SAFETY: We have checked that `falcon_ucode_offset + size_of::<FalconUCodeDescV3>` is
> -        // within the bounds of `data`. Also, this data vector is from ROM, and the `data` field
> -        // in `BiosImageBase` is immutable after construction.
> -        Ok(unsafe {
> +        let v2 = unsafe {
> +            &*(self
> +                .base
> +                .data
> +                .as_ptr()
> +                .add(falcon_ucode_offset)
> +                .cast::<FalconUCodeDescV2>())
> +        };
> +
> +        let v3 = unsafe {
>              &*(self
>                  .base
>                  .data
>                  .as_ptr()
>                  .add(falcon_ucode_offset)
>                  .cast::<FalconUCodeDescV3>())
> -        })
> +        };
> +
> +        // Return a FalconUCodeDesc structure.
> +        //
> +        // SAFETY: We have checked that `falcon_ucode_offset + size_of::<FalconUCodeDesc>` is
> +        // within the bounds of `data`. Also, this data vector is from ROM, and the `data` field
> +        // in `BiosImageBase` is immutable after construction.
> +        match ver {
> +            2 => Ok(FalconUCodeDesc::V2(v2.clone())),
> +            3 => Ok(FalconUCodeDesc::V3(v3.clone())),
> +            _ => Err(EINVAL),
> +        }
>      }
>  
>      /// Get the ucode data as a byte slice
> -    pub(crate) fn ucode(&self, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
> +    pub(crate) fn ucode(&self, desc: &FalconUCodeDesc) -> Result<&[u8]> {
>          let falcon_ucode_offset = self.falcon_ucode_offset;
>  
>          // The ucode data follows the descriptor.
>          let ucode_data_offset = falcon_ucode_offset + desc.size();
> -        let size = usize::from_safe_cast(desc.imem_load_size + desc.dmem_load_size);
> +        let size = usize::from_safe_cast(desc.imem_load_size() + desc.dmem_load_size());
>  
>          // Get the data slice, checking bounds in a single operation.
>          self.base
> @@ -1066,10 +1086,14 @@ pub(crate) fn ucode(&self, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
>      }
>  
>      /// Get the signatures as a byte slice
> -    pub(crate) fn sigs(&self, desc: &FalconUCodeDescV3) -> Result<&[Bcrt30Rsa3kSignature]> {
> +    pub(crate) fn sigs(&self, desc: &FalconUCodeDesc) -> Result<&[Bcrt30Rsa3kSignature]> {
> +        let hdr_size = match desc {
> +            FalconUCodeDesc::V2(_v2) => core::mem::size_of::<FalconUCodeDescV2>(),
> +            FalconUCodeDesc::V3(_v3) => core::mem::size_of::<FalconUCodeDescV3>(),
> +        };
>          // The signatures data follows the descriptor.
> -        let sigs_data_offset = self.falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>();
> -        let sigs_count = usize::from(desc.signature_count);
> +        let sigs_data_offset = self.falcon_ucode_offset + hdr_size;
> +        let sigs_count = usize::from(desc.signature_count());
>          let sigs_size = sigs_count * core::mem::size_of::<Bcrt30Rsa3kSignature>();
>  
>          // Make sure the data is within bounds.
> -- 
> 2.51.2
>