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
