On Thu Dec 18, 2025 at 12:29 PM JST, Timur Tabi wrote:
<snip>
> +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,
> + }
> + }
> +
> /// Returns the size in bytes of the header.
> pub(crate) fn size(&self) -> usize {
> + let hdr = self.as_descriptor().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 {
> + self.as_descriptor().imem_load_size()
> + }
> +
> + pub(crate) fn interface_offset(&self) -> u32 {
> + self.as_descriptor().interface_offset()
> + }
> +
> + pub(crate) fn dmem_load_size(&self) -> u32 {
> + self.as_descriptor().dmem_load_size()
> + }
> +
> + pub(crate) fn pkc_data_offset(&self) -> u32 {
> + self.as_descriptor().pkc_data_offset()
> + }
> +
> + pub(crate) fn engine_id_mask(&self) -> u16 {
> + self.as_descriptor().engine_id_mask()
> + }
> +
> + pub(crate) fn ucode_id(&self) -> u8 {
> + self.as_descriptor().ucode_id()
> + }
> +
> + pub(crate) fn signature_count(&self) -> u8 {
> + self.as_descriptor().signature_count()
> + }
>
> - ((self.hdr & HDR_SIZE_MASK) >> HDR_SIZE_SHIFT).into_safe_cast()
> + pub(crate) fn signature_versions(&self) -> u16 {
> + self.as_descriptor().signature_versions()
> }
> }
I wanted to make this comment on v3, but took the time to write some
code to validate the idea so this has slipped to this version. :)
This whole impl block becomes unneeded if you leverage `Deref` as a
replacement for `as_descriptor`:
impl Deref for FalconUCodeDesc {
type Target = dyn FalconUCodeDescriptor;
fn deref(&self) -> &Self::Target {
match self {
FalconUCodeDesc::V2(v2) => v2,
FalconUCodeDesc::V3(v3) => v3,
}
}
}
What this does is that it makes all the methods of
`FalconUCodeDescriptor` available automagically whenever the `.`
dereference operator it used, which is basically what these proxy
methods did.
Then the only remaining method, `size`, can be moved as a default method
of `FalconUCodeDescriptor` itself since all it does is call other
methods from that trait.
>
> 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(),
> }
For `brom_params` you are calling the virtual methods of
`FalconUCodeDescriptor`, but on the other methods you are doing a match.
I guess the reason is that using virtual methods all the way would make
`FalconUCodeDescriptor` grow some more, but still we would like to avoid
the match here.
You can achieve this by adding the following methods to
`FalconUCodeDescriptor`:
fn imem_sec_load_params(&self) -> FalconLoadTarget;
fn imem_ns_load_params(&self) -> Option<FalconLoadTarget>;
fn dmem_load_params(&self) -> FalconLoadTarget;
fn brom_params(&self) -> FalconBromParams;
And implementing them appropriately for the V2 and V3 headers. Then the
methods of the impl block above can just become:
fn imem_sec_load_params(&self) -> FalconLoadTarget {
self.desc.imem_sec_load_params()
}
fn imem_ns_load_params(&self) -> Option<FalconLoadTarget> {
self.desc.imem_ns_load_params()
}
fn dmem_load_params(&self) -> FalconLoadTarget {
self.desc.dmem_load_params()
}
fn brom_params(&self) -> FalconBromParams {
self.desc.brom_params()
}
