On 1/2/26 8:59 PM, Timur Tabi wrote:
...
> +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:
diff --git a/drivers/gpu/nova-core/firmware.rs
b/drivers/gpu/nova-core/firmware.rs
index 9f0ad02fbe22..c499491c25ab 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -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()
}
}
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs
b/drivers/gpu/nova-core/firmware/fwsec.rs
index f1f8f921aa43..28a52e251c3e 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -334,10 +334,12 @@ fn dmem_load_params(&self) -> FalconLoadTarget {
}
fn brom_params(&self) -> FalconBromParams {
+ // V2 descriptors don't have BROM parameters; use zero defaults.
+ // The Turing HAL's program_brom() is a no-op, so these values are
unused.
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().unwrap_or(0),
+ engine_id_mask: self.desc.engine_id_mask().unwrap_or(0),
+ ucode_id: self.desc.ucode_id().unwrap_or(0),
}
}
@@ -463,52 +465,64 @@ pub(crate) fn new(
) -> Result<Self> {
let ucode_dma = FirmwareDmaObject::<Self, _>::new_fwsec(dev, bios,
cmd)?;
- // Patch signature if needed.
+ // Patch signature if needed. V3 descriptors have signature metadata;
+ // V2 descriptors (Turing, GA100) don't require signature patching
here.
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 reg_fuse_version =
- falcon.signature_reg_fuse_version(bar, desc.engine_id_mask(),
desc.ucode_id())?;
- dev_dbg!(
- dev,
- "desc_sig_versions: {:#x}, reg_fuse_version: {}\n",
- desc_sig_versions,
- reg_fuse_version
- );
- let signature_idx = {
- let reg_fuse_version_bit = 1 << reg_fuse_version;
-
- // Check if the fuse version is supported by the firmware.
- if desc_sig_versions & reg_fuse_version_bit == 0 {
- dev_err!(
- dev,
- "no matching signature: {:#x} {:#x}\n",
- reg_fuse_version_bit,
- desc_sig_versions,
- );
- return Err(EINVAL);
- }
-
- // `desc_sig_versions` has one bit set per included signature.
Thus, the index of
- // the signature to patch is the number of bits in
`desc_sig_versions` set to `1`
- // before `reg_fuse_version_bit`.
-
- // Mask of the bits of `desc_sig_versions` to preserve.
- let reg_fuse_version_mask =
reg_fuse_version_bit.wrapping_sub(1);
-
- usize::from_safe_cast((desc_sig_versions &
reg_fuse_version_mask).count_ones())
- };
+ let ucode_signed = if let Some(sig_count) = desc.signature_count() {
+ if sig_count == 0 {
+ ucode_dma.no_patch_signature()
+ } else {
+ // These fields are guaranteed to be Some for V3 descriptors.
+ let pkc_data_offset = desc.pkc_data_offset().ok_or(EINVAL)?;
+ let engine_id_mask = desc.engine_id_mask().ok_or(EINVAL)?;
+ let ucode_id = desc.ucode_id().ok_or(EINVAL)?;
+ let sig_versions = desc.signature_versions().ok_or(EINVAL)?;
+
+ let sig_base_img =
+ usize::from_safe_cast(desc.imem_load_size() +
pkc_data_offset);
+ let desc_sig_versions = u32::from(sig_versions);
+ let reg_fuse_version =
+ falcon.signature_reg_fuse_version(bar, engine_id_mask,
ucode_id)?;
+ dev_dbg!(
+ dev,
+ "desc_sig_versions: {:#x}, reg_fuse_version: {}\n",
+ desc_sig_versions,
+ reg_fuse_version
+ );
+ let signature_idx = {
+ let reg_fuse_version_bit = 1 << reg_fuse_version;
+
+ // Check if the fuse version is supported by the firmware.
+ if desc_sig_versions & reg_fuse_version_bit == 0 {
+ dev_err!(
+ dev,
+ "no matching signature: {:#x} {:#x}\n",
+ reg_fuse_version_bit,
+ desc_sig_versions,
+ );
+ return Err(EINVAL);
+ }
+
+ // `desc_sig_versions` has one bit set per included
signature. Thus, the index
+ // of the signature to patch is the number of bits in
`desc_sig_versions` set
+ // to `1` before `reg_fuse_version_bit`.
+
+ // Mask of the bits of `desc_sig_versions` to preserve.
+ let reg_fuse_version_mask =
reg_fuse_version_bit.wrapping_sub(1);
+
+ usize::from_safe_cast((desc_sig_versions &
reg_fuse_version_mask).count_ones())
+ };
- dev_dbg!(dev, "patching signature with index {}\n", signature_idx);
- let signature = bios
- .fwsec_image()
- .sigs(&desc)
- .and_then(|sigs| sigs.get(signature_idx).ok_or(EINVAL))?;
+ dev_dbg!(dev, "patching signature with index {}\n",
signature_idx);
+ let signature = bios
+ .fwsec_image()
+ .sigs(&desc)
+ .and_then(|sigs| sigs.get(signature_idx).ok_or(EINVAL))?;
- ucode_dma.patch_signature(signature, sig_base_img)?
+ ucode_dma.patch_signature(signature, sig_base_img)?
+ }
} else {
+ // V2 descriptor - no signature patching needed
ucode_dma.no_patch_signature()
};
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 12256e5c8dc5..349b994fef92 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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,
--
John Hubbard
> +
> +/// 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..1c1dcdacf5f5 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 7c26e4a2d61c..12256e5c8dc5 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,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.
