Joel Fernandes
2025-Nov-17 23:10 UTC
[PATCH 09/11] gpu: nova-core: add FalconUCodeDescV2 support
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 at 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 >
Alexandre Courbot
2025-Nov-18 13:04 UTC
[PATCH 09/11] gpu: nova-core: add FalconUCodeDescV2 support
On Tue Nov 18, 2025 at 8:10 AM JST, Joel Fernandes wrote:> 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 at 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(), > } > } > }On principle, I tend to agree. In practice, we will probably never have more than these two variants, so we need to balance the benefit of a trait against the overhead of defining it in the first place (there are quite a few methods in there). Trait objects come with their own complications, i.e. you need to store them on the heap if you need more than a short-lived reference - but in our case the short-lived reference should be what we need anyway.