On Wed, May 21, 2025 at 03:45:11PM +0900, Alexandre Courbot wrote:
> +impl Vbios {
<snip>
> + pub(crate) fn fwsec_header(&self, pdev: &device::Device) ->
> Result<&FalconUCodeDescV3> {
> + self.fwsec_image.fwsec_header(pdev)
> + }
> +
> + pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]>
> {
> + self.fwsec_image.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
> + }
> +
> + pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
> + self.fwsec_image.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
> + }
Can't we just implement Deref here? Why do we need this indirection?
> +impl PcirStruct {
> + fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
> + if data.len() < core::mem::size_of::<PcirStruct>() {
> + dev_err!(pdev.as_ref(), "Not enough data for PcirStruct\n");
> + return Err(EINVAL);
> + }
> +
> + let mut signature = [0u8; 4];
> + signature.copy_from_slice(&data[0..4]);
> +
> + // Signature should be "PCIR" (0x52494350) or "NPDS" (0x5344504e)
> + if &signature != b"PCIR" && &signature != b"NPDS" {
> + dev_err!(
> + pdev.as_ref(),
> + "Invalid signature for PcirStruct: {:?}\n",
> + signature
> + );
> + return Err(EINVAL);
> + }
> +
> + let mut class_code = [0u8; 3];
> + class_code.copy_from_slice(&data[13..16]);
> +
> + Ok(PcirStruct {
> + signature,
> + vendor_id: u16::from_le_bytes([data[4], data[5]]),
> + device_id: u16::from_le_bytes([data[6], data[7]]),
> + device_list_ptr: u16::from_le_bytes([data[8], data[9]]),
> + pci_data_struct_len: u16::from_le_bytes([data[10], data[11]]),
> + pci_data_struct_rev: data[12],
> + class_code,
> + image_len: u16::from_le_bytes([data[16], data[17]]),
> + vendor_rom_rev: u16::from_le_bytes([data[18], data[19]]),
> + code_type: data[20],
> + last_image: data[21],
> + max_runtime_image_len: u16::from_le_bytes([data[22], data[23]]),
> + })
> + }
> +
> + /// Check if this is the last image in the ROM
> + fn is_last(&self) -> bool {
> + self.last_image & LAST_IMAGE_BIT_MASK != 0
> + }
> +
> + /// Calculate image size in bytes
> + fn image_size_bytes(&self) -> Result<usize> {
> + if self.image_len > 0 {
Please make this check when creating the structure...
> + // Image size is in 512-byte blocks
...and make this a type invariant.
> + Ok(self.image_len as usize * 512)
It should also be a type invariant that this does not overflow.
The same applies to NpdeStruct.
> + } else {
> + Err(EINVAL)
> + }
> + }
> +}
<snip>
> + /// Try to find NPDE in the data, the NPDE is right after the PCIR.
> + fn find_in_data(
> + pdev: &pci::Device,
> + data: &[u8],
> + rom_header: &PciRomHeader,
> + pcir: &PcirStruct,
> + ) -> Option<Self> {
> + // Calculate the offset where NPDE might be located
> + // NPDE should be right after the PCIR structure, aligned to 16 bytes
> + let pcir_offset = rom_header.pci_data_struct_offset as usize;
> + let npde_start = (pcir_offset + pcir.pci_data_struct_len as usize +
> 0x0F) & !0x0F;
What's this magic offset and mask?
> +
> + // Check if we have enough data
> + if npde_start + 11 > data.len() {
'+ 11'?
> + dev_err!(pdev.as_ref(), "Not enough data for NPDE\n");
BiosImageBase declares this as "NVIDIA PCI Data Extension (optional)". If it's
really optional, why is this an error?
> + return None;
> + }
> +
> + // Try to create NPDE from the data
> + NpdeStruct::new(pdev, &data[npde_start..])
> + .inspect_err(|e| {
> + dev_err!(pdev.as_ref(), "Error creating NpdeStruct: {:?}\n",
> e);
> + })
> + .ok()
So, this returns None if it's a real error. This indicates that the return type
should just be Result<Option<Self>>.
> +struct FwSecBiosPartial {
Since this structure follows the builder pattern, can we please call it
FwSecBiosBuilder?
> + base: BiosImageBase,
> + // FWSEC-specific fields
> + // These are temporary fields that are used during the construction of
> + // the FwSecBiosPartial. Once FwSecBiosPartial is constructed, the
> + // falcon_ucode_offset will be copied into a new FwSecBiosImage.
> +
> + // The offset of the Falcon data from the start of Fwsec image
> + falcon_data_offset: Option<usize>,
> + // The PmuLookupTable starts at the offset of the falcon data pointer
> + pmu_lookup_table: Option<PmuLookupTable>,
> + // The offset of the Falcon ucode
> + falcon_ucode_offset: Option<usize>,
> +}
> +
> +struct FwSecBiosImage {
> + base: BiosImageBase,
> + // The offset of the Falcon ucode
> + falcon_ucode_offset: usize,
> +}
> +
> +// Convert from BiosImageBase to BiosImage
> +impl TryFrom<BiosImageBase> for BiosImage {
Why is this a TryFrom impl, instead of a regular constructor, i.e.
BiosImage::new()?
I don't think this is a canonical conversion.
> + type Error = Error;
> +
> + fn try_from(base: BiosImageBase) -> Result<Self> {
> + match base.pcir.code_type {
> + 0x00 => Ok(BiosImage::PciAt(base.try_into()?)),
> + 0x03 => Ok(BiosImage::Efi(EfiBiosImage { base })),
> + 0x70 => Ok(BiosImage::Nbsi(NbsiBiosImage { base })),
> + 0xE0 => Ok(BiosImage::FwSecPartial(FwSecBiosPartial {
> + base,
> + falcon_data_offset: None,
> + pmu_lookup_table: None,
> + falcon_ucode_offset: None,
> + })),
> + _ => Err(EINVAL),
> + }
> + }
> +}
<snip>
> +impl TryFrom<BiosImageBase> for PciAtBiosImage {
Same here.
> + type Error = Error;
> +
> + fn try_from(base: BiosImageBase) -> Result<Self> {
> + let data_slice = &base.data;
> + let (bit_header, bit_offset) =
> PciAtBiosImage::find_bit_header(data_slice)?;
> +
> + Ok(PciAtBiosImage {
> + base,
> + bit_header,
> + bit_offset,
> + })
> + }
> +}
<snip>
> +impl FwSecBiosImage {
> + fn new(pdev: &pci::Device, data: FwSecBiosPartial) -> Result<Self> {
Please add a method FwSecBiosBuilder::build() that returns an instance of
FwSecBiosImage instead.
