Message-ID: <9de086d3-e441-439f-9ff5-aa66a99643ba@nvidia.com>
Date: Thu, 5 Jun 2025 12:09:46 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Lyude Paul <lyude@...hat.com>, Alexandre Courbot <acourbot@...dia.com>,
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>
Cc: John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>,
 Timur Tabi <ttabi@...dia.com>, Alistair Popple <apopple@...dia.com>,
 linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
 nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
 Shirish Baskaran <sbaskaran@...dia.com>
Subject: Re: [PATCH v4 16/20] nova-core: Add support for VBIOS ucode
 extraction for boot

Hi Lyude,

>> +bios_image! {
>> +    PciAt PciAtBiosImage,   // PCI-AT compatible BIOS image
>> +    Efi EfiBiosImage,       // EFI (Extensible Firmware Interface)
>> +    Nbsi NbsiBiosImage,     // NBSI (Nvidia Bios System Interface)
>> +    FwSecPartial FwSecBiosPartial, // FWSEC (Firmware Security)
>> +}
> 
> Maybe add a colon to separate the two fields in this macro so it looks more
> like a struct declaration?

Sure, will do.

> 
>> +
>> +struct PciAtBiosImage {
>> +    base: BiosImageBase,
>> +    bit_header: BitHeader,
>> +    bit_offset: usize,
>> +}
>> +
>> +struct EfiBiosImage {
>> +    base: BiosImageBase,
>> +    // EFI-specific fields can be added here in the future.
>> +}
>> +
>> +struct NbsiBiosImage {
>> +    base: BiosImageBase,
>> +    // NBSI-specific fields can be added here in the future.
>> +}
>> +
>> +struct FwSecBiosPartial {
>> +    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>,
> 
> Shouldn't these last 3 comments be docstrings?

Sure, will change.

> 
>> +}
>> +
>> +struct FwSecBiosImage {
>> +    base: BiosImageBase,
>> +    // The offset of the Falcon ucode
> 
> Same here

Sure, will change.

> 
>> +    falcon_ucode_offset: usize,
>> +}
>> +
>> +// Convert from BiosImageBase to BiosImage
>> +impl TryFrom<BiosImageBase> for BiosImage {
>> +    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),
>> +        }
>> +    }
>> +}
>> +
>> +/// BIOS Image structure containing various headers and references
>> +/// fields base to all BIOS images. Each BiosImage type has a
>> +/// BiosImageBase type along with other image-specific fields.
>> +/// Note that Rust favors composition of types over inheritance.
>> +#[derive(Debug)]
>> +#[expect(dead_code)]
>> +struct BiosImageBase {
>> +    /// PCI ROM Expansion Header
>> +    rom_header: PciRomHeader,
>> +    /// PCI Data Structure
>> +    pcir: PcirStruct,
>> +    /// NVIDIA PCI Data Extension (optional)
>> +    npde: Option<NpdeStruct>,
>> +    /// Image data (includes ROM header and PCIR)
>> +    data: KVec<u8>,
>> +}
>> +
>> +impl BiosImageBase {
>> +    fn into_image(self) -> Result<BiosImage> {
>> +        BiosImage::try_from(self)
>> +    }
>> +
>> +    /// Creates a new BiosImageBase from raw byte data.
>> +    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>> +        // Ensure we have enough data for the ROM header
>> +        if data.len() < 26 {
>> +            dev_err!(pdev.as_ref(), "Not enough data for ROM header\n");
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // Parse the ROM header
>> +        let rom_header = PciRomHeader::new(pdev, &data[0..26])
>> +            .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to create PciRomHeader: {:?}\n", e))?;
>> +
>> +        // Get the PCI Data Structure using the pointer from the ROM header
>> +        let pcir_offset = rom_header.pci_data_struct_offset as usize;
>> +        let pcir_data = data
>> +            .get(pcir_offset..pcir_offset + core::mem::size_of::<PcirStruct>())
>> +            .ok_or(EINVAL)
>> +            .inspect_err(|_| {
>> +                dev_err!(
>> +                    pdev.as_ref(),
>> +                    "PCIR offset {:#x} out of bounds (data length: {})\n",
>> +                    pcir_offset,
>> +                    data.len()
>> +                );
>> +                dev_err!(
>> +                    pdev.as_ref(),
>> +                    "Consider reading more data for construction of BiosImage\n"
>> +                );
>> +            })?;
>> +
>> +        let pcir = PcirStruct::new(pdev, pcir_data)
>> +            .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to create PcirStruct: {:?}\n", e))?;
>> +
>> +        // Look for NPDE structure if this is not an NBSI image (type != 0x70)
>> +        let npde = NpdeStruct::find_in_data(pdev, data, &rom_header, &pcir);
>> +
>> +        // Create a copy of the data
>> +        let mut data_copy = KVec::new();
>> +        data_copy.extend_with(data.len(), 0, GFP_KERNEL)?;
>> +        data_copy.copy_from_slice(data);
>> +
>> +        Ok(BiosImageBase {
>> +            rom_header,
>> +            pcir,
>> +            npde,
>> +            data: data_copy,
>> +        })
>> +    }
>> +}
>> +
>> +/// The PciAt BIOS image is typically the first BIOS image type found in the
>> +/// BIOS image chain. It contains the BIT header and the BIT tokens.
>> +impl PciAtBiosImage {
>> +    /// Find a byte pattern in a slice
>> +    fn find_byte_pattern(haystack: &[u8], needle: &[u8]) -> Result<usize> {
>> +        haystack
>> +            .windows(needle.len())
>> +            .position(|window| window == needle)
>> +            .ok_or(EINVAL)
>> +    }
>> +
>> +    /// Find the BIT header in the PciAtBiosImage
>> +    fn find_bit_header(data: &[u8]) -> Result<(BitHeader, usize)> {
>> +        let bit_pattern = [0xff, 0xb8, b'B', b'I', b'T', 0x00];
>> +        let bit_offset = Self::find_byte_pattern(data, &bit_pattern)?;
>> +        let bit_header = BitHeader::new(&data[bit_offset..])?;
>> +
>> +        Ok((bit_header, bit_offset))
>> +    }
>> +
>> +    /// Get a BIT token entry from the BIT table in the PciAtBiosImage
>> +    fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
>> +        BitToken::from_id(self, token_id)
>> +    }
>> +
>> +    /// Find the Falcon data pointer structure in the PciAtBiosImage
>> +    /// This is just a 4 byte structure that contains a pointer to the
>> +    /// Falcon data in the FWSEC image.
>> +    fn falcon_data_ptr(&self, pdev: &pci::Device) -> Result<u32> {
>> +        let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
>> +
>> +        // Make sure we don't go out of bounds
>> +        if token.data_offset as usize + 4 > self.base.data.len() {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // read the 4 bytes at the offset specified in the token
>> +        let offset = token.data_offset as usize;
>> +        let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
>> +            dev_err!(pdev.as_ref(), "Failed to convert data slice to array");
>> +            EINVAL
>> +        })?;
>> +
>> +        let data_ptr = u32::from_le_bytes(bytes);
>> +
>> +        if (data_ptr as usize) < self.base.data.len() {
>> +            dev_err!(pdev.as_ref(), "Falcon data pointer out of bounds\n");
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        Ok(data_ptr)
> 
> Not 100% sure about this but maybe this should be data_offset and not
> data_ptr? It took me a bit to understand what was going on here since normally
> you can't tell if a pointer is valid just by comparing it to the raw length of
> a piece of data
> 

I understand it is a bit confusing, but is consistent with OpenRM code base
naming so I'll like to leave it alone for consistency.

>> +    }
>> +}
>> +
>> +impl TryFrom<BiosImageBase> for PciAtBiosImage {
>> +    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,
>> +        })
>> +    }
>> +}
>> +
>> +/// The PmuLookupTableEntry structure is a single entry in the PmuLookupTable.
>> +/// See the PmuLookupTable description for more information.
>> +#[expect(dead_code)]
>> +struct PmuLookupTableEntry {
>> +    application_id: u8,
>> +    target_id: u8,
>> +    data: u32,
>> +}
>> +
>> +impl PmuLookupTableEntry {
>> +    fn new(data: &[u8]) -> Result<Self> {
>> +        if data.len() < 5 {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        Ok(PmuLookupTableEntry {
>> +            application_id: data[0],
>> +            target_id: data[1],
>> +            data: u32::from_le_bytes(data[2..6].try_into().map_err(|_| EINVAL)?),
>> +        })
>> +    }
>> +}
>> +
>> +/// The PmuLookupTableEntry structure is used to find the PmuLookupTableEntry
>> +/// for a given application ID. The table of entries is pointed to by the falcon
>> +/// data pointer in the BIT table, and is used to locate the Falcon Ucode.
>> +#[expect(dead_code)]
>> +struct PmuLookupTable {
>> +    version: u8,
>> +    header_len: u8,
>> +    entry_len: u8,
>> +    entry_count: u8,
>> +    table_data: KVec<u8>,
>> +}
>> +
>> +impl PmuLookupTable {
>> +    fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
>> +        if data.len() < 4 {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let header_len = data[1] as usize;
>> +        let entry_len = data[2] as usize;
>> +        let entry_count = data[3] as usize;
>> +
>> +        let required_bytes = header_len + (entry_count * entry_len);
>> +
>> +        if data.len() < required_bytes {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "PmuLookupTable data length less than required\n"
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // Create a copy of only the table data
>> +        let table_data = {
>> +            let mut ret = KVec::new();
>> +            ret.extend_from_slice(&data[header_len..required_bytes], GFP_KERNEL)?;
>> +            ret
>> +        };
>> +
>> +        // Debug logging of entries (dumps the table data to dmesg)
>> +        if cfg!(debug_assertions) {
>> +            for i in (header_len..required_bytes).step_by(entry_len) {
>> +                dev_dbg!(
>> +                    pdev.as_ref(),
>> +                    "PMU entry: {:02x?}\n",
>> +                    &data[i..][..entry_len]
>> +                );
>> +            }
>> +        }
> 
> Not sure this makes sense - debug_assertions is supposed to be about
> assertions, we probably shouldn't try to use it for other things (especially
> since we've already got dev_dbg! here)

This was suggested by Danilo. I don't really feel strongly either way, IMO I am
also Ok with running it in production.

> 
>> +
>> +        Ok(PmuLookupTable {
>> +            version: data[0],
>> +            header_len: header_len as u8,
>> +            entry_len: entry_len as u8,
>> +            entry_count: entry_count as u8,
>> +            table_data,
>> +        })
>> +    }
>> +
>> +    fn lookup_index(&self, idx: u8) -> Result<PmuLookupTableEntry> {
>> +        if idx >= self.entry_count {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let index = (idx as usize) * self.entry_len as usize;
>> +        PmuLookupTableEntry::new(&self.table_data[index..])
>> +    }
>> +
>> +    // find entry by type value
>> +    fn find_entry_by_type(&self, entry_type: u8) -> Result<PmuLookupTableEntry> {
>> +        for i in 0..self.entry_count {
>> +            let entry = self.lookup_index(i)?;
>> +            if entry.application_id == entry_type {
>> +                return Ok(entry);
>> +            }
>> +        }
>> +
>> +        Err(EINVAL)
>> +    }
>> +}
>> +
>> +/// The FwSecBiosImage structure contains the PMU table and the Falcon Ucode.
>> +/// The PMU table contains voltage/frequency tables as well as a pointer to the
>> +/// Falcon Ucode.
>> +impl FwSecBiosPartial {
>> +    fn setup_falcon_data(
>> +        &mut self,
>> +        pdev: &pci::Device,
>> +        pci_at_image: &PciAtBiosImage,
>> +        first_fwsec: &FwSecBiosPartial,
>> +    ) -> Result {
>> +        let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
>> +        let mut pmu_in_first_fwsec = false;
>> +
>> +        // The falcon data pointer assumes that the PciAt and FWSEC images
>> +        // are contiguous in memory. However, testing shows the EFI image sits in
>> +        // between them. So calculate the offset from the end of the PciAt image
>> +        // rather than the start of it. Compensate.
>> +        offset -= pci_at_image.base.data.len();
>> +
>> +        // The offset is now from the start of the first Fwsec image, however
>> +        // the offset points to a location in the second Fwsec image. Since
>> +        // the fwsec images are contiguous, subtract the length of the first Fwsec
>> +        // image from the offset to get the offset to the start of the second
>> +        // Fwsec image.
>> +        if offset < first_fwsec.base.data.len() {
>> +            pmu_in_first_fwsec = true;
>> +        } else {
>> +            offset -= first_fwsec.base.data.len();
>> +        }
>> +
>> +        self.falcon_data_offset = Some(offset);
>> +
>> +        if pmu_in_first_fwsec {
>> +            self.pmu_lookup_table =
>> +                Some(PmuLookupTable::new(pdev, &first_fwsec.base.data[offset..])?);
>> +        } else {
>> +            self.pmu_lookup_table = Some(PmuLookupTable::new(pdev, &self.base.data[offset..])?);
>> +        }
>> +
>> +        match self
>> +            .pmu_lookup_table
>> +            .as_ref()
>> +            .ok_or(EINVAL)?
>> +            .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
>> +        {
>> +            Ok(entry) => {
>> +                let mut ucode_offset = entry.data as usize;
>> +                ucode_offset -= pci_at_image.base.data.len();
>> +                if ucode_offset < first_fwsec.base.data.len() {
>> +                    dev_err!(pdev.as_ref(), "Falcon Ucode offset not in second Fwsec.\n");
>> +                    return Err(EINVAL);
>> +                }
>> +                ucode_offset -= first_fwsec.base.data.len();
>> +                self.falcon_ucode_offset = Some(ucode_offset);
>> +            }
>> +            Err(e) => {
>> +                dev_err!(
>> +                    pdev.as_ref(),
>> +                    "PmuLookupTableEntry not found, error: {:?}\n",
>> +                    e
>> +                );
>> +                return Err(EINVAL);
>> +            }
>> +        }
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl FwSecBiosImage {
>> +    fn new(pdev: &pci::Device, data: FwSecBiosPartial) -> Result<Self> {
>> +        let ret = FwSecBiosImage {
>> +            base: data.base,
>> +            falcon_ucode_offset: data.falcon_ucode_offset.ok_or(EINVAL)?,
>> +        };
>> +
>> +        if cfg!(debug_assertions) {
>> +            // Print the desc header for debugging
>> +            let desc = ret.fwsec_header(pdev.as_ref())?;
>> +            dev_dbg!(pdev.as_ref(), "PmuLookupTableEntry desc: {:#?}\n", desc);
>> +        }
> 
> Again - definitely don't think we should be using debug_assertions for this

Same comment reply above.

>> +
>> +        Ok(ret)
>> +    }
>> +
>> +    /// Get the FwSec header (FalconUCodeDescV3)
>> +    fn fwsec_header(&self, dev: &device::Device) -> Result<&FalconUCodeDescV3> {
>> +        // 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!(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()
>> +            .map_err(|_| EINVAL)?;
>> +        let hdr = u32::from_le_bytes(hdr_bytes);
>> +        let ver = (hdr & 0xff00) >> 8;
>> +
>> +        if ver != 3 {
>> +            dev_err!(dev, "invalid fwsec firmware version: {:?}\n", ver);
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // Return a reference to the FalconUCodeDescV3 structure SAFETY: we have checked that
>> +        // `falcon_ucode_offset + size_of::<FalconUCodeDescV3` is within the bounds of `data.`
> 
> The SAFETY comment here should start on its own line in the comment

Fixed.

> 
>> +        Ok(unsafe {
>> +            &*(self.base.data.as_ptr().add(falcon_ucode_offset) as *const FalconUCodeDescV3)
> 
> FWIW: I would use cast here, not as.

Ok, I'll kindly defer this to Alex since he wrote this line.

> Also though, I think you need to justify> in the safety comment here why it's
safe to be able to hold an immutable
> reference (e.g. why can we expect this data not to be mutated for the lifetime
> of the reference?)

This data vector is ROM, also 'data' in BiosImageBase is immutable after
construction. I'll update the comment.

> 
>> +        })
>> +    }
> 
> ^ missing a newline here

Fixed.

> 
>> +    /// Get the ucode data as a byte slice
>> +    fn fwsec_ucode(&self, dev: &device::Device, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
>> +        let falcon_ucode_offset = self.falcon_ucode_offset;
> 
> I think we can drop this variable if we're only calling falcon_ucode_offset
> once
> 

Good point, fixed.

>> +
>> +        // The ucode data follows the descriptor
>> +        let ucode_data_offset = falcon_ucode_offset + desc.size();
>> +        let size = (desc.imem_load_size + desc.dmem_load_size) as usize;
>> +
>> +        // Get the data slice, checking bounds in a single operation
>> +        self.base
>> +            .data
>> +            .get(ucode_data_offset..ucode_data_offset + size)
>> +            .ok_or(ERANGE)
>> +            .inspect_err(|_| dev_err!(dev, "fwsec ucode data not contained within BIOS bounds\n"))
>> +    }
>> +
>> +    /// Get the signatures as a byte slice
>> +    fn fwsec_sigs(&self, dev: &device::Device, desc: &FalconUCodeDescV3) -> Result<&[u8]> {
>> +        const SIG_SIZE: usize = 96 * 4;
>> +
>> +        let falcon_ucode_offset = self.falcon_ucode_offset;
>> +
>> +        // The signatures data follows the descriptor
>> +        let sigs_data_offset = falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>();
>> +        let size = desc.signature_count as usize * SIG_SIZE;
>> +
>> +        // Make sure the data is within bounds
>> +        if sigs_data_offset + size > self.base.data.len() {
>> +            dev_err!(
>> +                dev,
>> +                "fwsec signatures data not contained within BIOS bounds\n"
>> +            );
>> +            return Err(ERANGE);
>> +        }
>> +
>> +        Ok(&self.base.data[sigs_data_offset..sigs_data_offset + size])
>> +    }
>> +}
>>
> 
> Would be nice to get other people's take on this but I feel like that we
> probably shouldn't make these methods conditional at this point,
> FwSecBiosImage as a type name with FwSecBiosPartial implies that we should
> have already figured out if it's a valid bios image and extracted the relevant
> data in ::new() right?
> 

Do you mean these things be computed at object construction time? I am not sure
if I agree with that, because maybe computing some of these will be optional in
the future. And it will also require additional fields/footprint in the struct
to store them. Probably more LOC too.

thanks,

 - Joel

Powered by blists - more mailing lists