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 <[email protected]>
> ---
> drivers/gpu/nova-core/firmware.rs | 108 +++++++++++++++++++++++-
> drivers/gpu/nova-core/firmware/fwsec.rs | 72 ++++++++++------
> drivers/gpu/nova-core/vbios.rs | 74 ++++++++++------
> 3 files changed, 202 insertions(+), 52 deletions(-)
>
> 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
>
