Re: [PATCH v4 16/20] nova-core: Add support for VBIOS ucode extraction for boot

[Joel Fernandes <joelagnelf@nvidia.com>]

On 6/2/2025 9:33 AM, Danilo Krummrich wrote:
[...]
>> +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.
> 

Done, that's a lot cleaner, thanks!

> 
>> +    /// 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?
> 

Good catch, me and Timur were just coincidentally talking about this as well!
Indeed it should not be an error since NPDE on some GPUs doesn't exist.

Will address the other NPDE comments separately since I have to do some research
first. Thanks for double checking.

>> +            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?

Yes, done.

>> +    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.

The main advantage is to use .try_into(). It also documents we're implementing a
conversion from one type to another. I am not sure where the boundary is, but
when you requested me to get rid the other TryFrom(s), I did that but I left
these ones alone because I'd like to use .try_into() for these. I think it makes
sense to convert a BiosImageBase to BiosImage since they're both essentially
similar. Alex, do you have any thoughts on it as you had suggested this for
other usecases during the initial nova-core stub series as well?

Btw, .try_into() does hurt readability a bit even though its more of a
short-hand, since one has to work harder to know what type converts to what, so
I'm really Ok either way here.

> 
>> +    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.

Same comment as above.

>> +    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.

Done, I made this change and it is cleaner. Thanks!

 - Joel