From: Joel Fernandes <joelagnelf@nvidia.com>
Add support for navigating and setting up vBIOS ucode data required for
GSP to boot. The main data extracted from the vBIOS is the FWSEC-FRTS
firmware which runs on the GSP processor. This firmware runs in high
secure mode, and sets up the WPR2 (Write protected region) before the
Booter runs on the SEC2 processor.
Also add log messages to show the BIOS images.
[102141.013287] NovaCore: Found BIOS image at offset 0x0, size: 0xfe00, type: PciAt
[102141.080692] NovaCore: Found BIOS image at offset 0xfe00, size: 0x14800, type: Efi
[102141.098443] NovaCore: Found BIOS image at offset 0x24600, size: 0x5600, type: FwSec
[102141.415095] NovaCore: Found BIOS image at offset 0x29c00, size: 0x60800, type: FwSec
Tested on my Ampere GA102 and boot is successful.
[applied changes by Alex Courbot for fwsec signatures]
[applied feedback from Alex Courbot and Timur Tabi]
[applied changes related to code reorg, prints etc from Danilo Krummrich]
[acourbot@nvidia.com: fix clippy warnings]
[acourbot@nvidia.com: remove now-unneeded Devres acquisition]
[acourbot@nvidia.com: fix read_more to read `len` bytes, not u32s]
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Shirish Baskaran <sbaskaran@nvidia.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Timur Tabi <ttabi@nvidia.com>
Cc: Ben Skeggs <bskeggs@nvidia.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/gpu/nova-core/firmware.rs | 2 -
drivers/gpu/nova-core/gpu.rs | 3 +
drivers/gpu/nova-core/nova_core.rs | 1 +
drivers/gpu/nova-core/vbios.rs | 1147 ++++++++++++++++++++++++++++++++++++
4 files changed, 1151 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index 1eb216307cd01d975b3d5beda1dc516f34b4b3f2..960982174d834c7c66a47ecfb3a15bf47116b2c5 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -80,8 +80,6 @@ pub(crate) struct FalconUCodeDescV3 {
_reserved: u16,
}
-// To be removed once that code is used.
-#[expect(dead_code)]
impl FalconUCodeDescV3 {
pub(crate) fn size(&self) -> usize {
((self.hdr & 0xffff0000) >> 16) as usize
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index ece13594fba687f3f714e255b5436e72d80dece3..4bf7f72247e5320935a517270b5a0e1ec2becfec 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -9,6 +9,7 @@
use crate::firmware::Firmware;
use crate::regs;
use crate::util;
+use crate::vbios::Vbios;
use core::fmt;
macro_rules! define_chipset {
@@ -238,6 +239,8 @@ pub(crate) fn new(
let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
+ let _bios = Vbios::new(pdev, bar)?;
+
Ok(pin_init!(Self {
spec,
bar: devres_bar,
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index 8342482a1aa16da2e69f7d99143c1549a82c969e..ff6d0b40c18f36af4c7e2d5c839fdf77dba23321 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -10,6 +10,7 @@
mod gpu;
mod regs;
mod util;
+mod vbios;
kernel::module_pci_driver! {
type: driver::NovaCore,
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
new file mode 100644
index 0000000000000000000000000000000000000000..cd55d8dbf8e12d532f776d7544c7e5f2a865d6f8
--- /dev/null
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -0,0 +1,1147 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! VBIOS extraction and parsing.
+
+// To be removed when all code is used.
+#![expect(dead_code)]
+
+use crate::driver::Bar0;
+use crate::firmware::FalconUCodeDescV3;
+use core::convert::TryFrom;
+use kernel::device;
+use kernel::error::Result;
+use kernel::num::NumAlign;
+use kernel::pci;
+use kernel::prelude::*;
+
+/// The offset of the VBIOS ROM in the BAR0 space.
+const ROM_OFFSET: usize = 0x300000;
+/// The maximum length of the VBIOS ROM to scan into.
+const BIOS_MAX_SCAN_LEN: usize = 0x100000;
+/// The size to read ahead when parsing initial BIOS image headers.
+const BIOS_READ_AHEAD_SIZE: usize = 1024;
+/// The bit in the last image indicator byte for the PCI Data Structure that
+/// indicates the last image. Bit 0-6 are reserved, bit 7 is last image bit.
+const LAST_IMAGE_BIT_MASK: u8 = 0x80;
+
+// PMU lookup table entry types. Used to locate PMU table entries
+// in the Fwsec image, corresponding to falcon ucodes.
+#[expect(dead_code)]
+const FALCON_UCODE_ENTRY_APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
+#[expect(dead_code)]
+const FALCON_UCODE_ENTRY_APPID_FWSEC_DBG: u8 = 0x45;
+const FALCON_UCODE_ENTRY_APPID_FWSEC_PROD: u8 = 0x85;
+
+/// Vbios Reader for constructing the VBIOS data
+struct VbiosIterator<'a> {
+ pdev: &'a pci::Device,
+ bar0: &'a Bar0,
+ // VBIOS data vector: As BIOS images are scanned, they are added to this vector
+ // for reference or copying into other data structures. It is the entire
+ // scanned contents of the VBIOS which progressively extends. It is used
+ // so that we do not re-read any contents that are already read as we use
+ // the cumulative length read so far, and re-read any gaps as we extend
+ // the length.
+ data: KVec<u8>,
+ current_offset: usize, // Current offset for iterator
+ last_found: bool, // Whether the last image has been found
+}
+
+impl<'a> VbiosIterator<'a> {
+ fn new(pdev: &'a pci::Device, bar0: &'a Bar0) -> Result<Self> {
+ Ok(Self {
+ pdev,
+ bar0,
+ data: KVec::new(),
+ current_offset: 0,
+ last_found: false,
+ })
+ }
+
+ /// Read bytes from the ROM at the current end of the data vector
+ fn read_more(&mut self, len: usize) -> Result {
+ let current_len = self.data.len();
+ let start = ROM_OFFSET + current_len;
+
+ // Ensure length is a multiple of 4 for 32-bit reads
+ if len % core::mem::size_of::<u32>() != 0 {
+ dev_err!(
+ self.pdev.as_ref(),
+ "VBIOS read length {} is not a multiple of 4\n",
+ len
+ );
+ return Err(EINVAL);
+ }
+
+ self.data.reserve(len, GFP_KERNEL)?;
+ // Read ROM data bytes and push directly to vector
+ for i in (0..len).step_by(core::mem::size_of::<u32>()) {
+ // Read 32-bit word from the VBIOS ROM
+ let word = self.bar0.try_read32(start + i)?;
+
+ // Convert the u32 to a 4 byte array and push each byte
+ word.to_ne_bytes()
+ .iter()
+ .try_for_each(|&b| self.data.push(b, GFP_KERNEL))?;
+ }
+
+ Ok(())
+ }
+
+ /// Read bytes at a specific offset, filling any gap
+ fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
+ if offset > BIOS_MAX_SCAN_LEN {
+ dev_err!(self.pdev.as_ref(), "Error: exceeded BIOS scan limit.\n");
+ return Err(EINVAL);
+ }
+
+ // If offset is beyond current data size, fill the gap first
+ let current_len = self.data.len();
+ let gap_bytes = offset.saturating_sub(current_len);
+
+ // Now read the requested bytes at the offset
+ self.read_more(gap_bytes + len)
+ }
+
+ /// Read a BIOS image at a specific offset and create a BiosImage from it.
+ /// self.data is extended as needed and a new BiosImage is returned.
+ /// @context is a string describing the operation for error reporting
+ fn read_bios_image_at_offset(
+ &mut self,
+ offset: usize,
+ len: usize,
+ context: &str,
+ ) -> Result<BiosImage> {
+ let data_len = self.data.len();
+ if offset + len > data_len {
+ self.read_more_at_offset(offset, len).inspect_err(|e| {
+ dev_err!(
+ self.pdev.as_ref(),
+ "Failed to read more at offset {:#x}: {:?}\n",
+ offset,
+ e
+ )
+ })?;
+ }
+
+ BiosImage::new(self.pdev, &self.data[offset..offset + len]).inspect_err(|err| {
+ dev_err!(
+ self.pdev.as_ref(),
+ "Failed to {} at offset {:#x}: {:?}\n",
+ context,
+ offset,
+ err
+ )
+ })
+ }
+}
+
+impl<'a> Iterator for VbiosIterator<'a> {
+ type Item = Result<BiosImage>;
+
+ /// Iterate over all VBIOS images until the last image is detected or offset
+ /// exceeds scan limit.
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.last_found {
+ return None;
+ }
+
+ if self.current_offset > BIOS_MAX_SCAN_LEN {
+ dev_err!(
+ self.pdev.as_ref(),
+ "Error: exceeded BIOS scan limit, stopping scan\n"
+ );
+ return None;
+ }
+
+ // Parse image headers first to get image size
+ let image_size = match self
+ .read_bios_image_at_offset(
+ self.current_offset,
+ BIOS_READ_AHEAD_SIZE,
+ "parse initial BIOS image headers",
+ )
+ .and_then(|image| image.image_size_bytes())
+ {
+ Ok(size) => size,
+ Err(e) => return Some(Err(e)),
+ };
+
+ // Now create a new BiosImage with the full image data
+ let full_image = match self.read_bios_image_at_offset(
+ self.current_offset,
+ image_size,
+ "parse full BIOS image",
+ ) {
+ Ok(image) => image,
+ Err(e) => return Some(Err(e)),
+ };
+
+ self.last_found = full_image.is_last();
+
+ // Advance to next image (aligned to 512 bytes)
+ self.current_offset += image_size;
+ self.current_offset = self.current_offset.align_up(512);
+
+ Some(Ok(full_image))
+ }
+}
+
+pub(crate) struct Vbios {
+ pub fwsec_image: Option<FwSecBiosImage>,
+}
+
+impl Vbios {
+ /// Probe for VBIOS extraction
+ /// Once the VBIOS object is built, bar0 is not read for vbios purposes anymore.
+ pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
+ // Images to extract from iteration
+ let mut pci_at_image: Option<PciAtBiosImage> = None;
+ let mut first_fwsec_image: Option<FwSecBiosImage> = None;
+ let mut second_fwsec_image: Option<FwSecBiosImage> = None;
+
+ // Parse all VBIOS images in the ROM
+ for image_result in VbiosIterator::new(pdev, bar0)? {
+ let full_image = image_result?;
+
+ dev_info!(
+ pdev.as_ref(),
+ "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
+ full_image.image_size_bytes()?,
+ full_image.image_type_str(),
+ full_image.is_last()
+ );
+
+ // Get references to images we will need after the loop, in order to
+ // setup the falcon data offset.
+ match full_image {
+ BiosImage::PciAt(image) => {
+ pci_at_image = Some(image);
+ }
+ BiosImage::FwSec(image) => {
+ if first_fwsec_image.is_none() {
+ first_fwsec_image = Some(image);
+ } else {
+ second_fwsec_image = Some(image);
+ }
+ }
+ // For now we don't need to handle these
+ BiosImage::Efi(_image) => {}
+ BiosImage::Nbsi(_image) => {}
+ }
+ }
+
+ // Using all the images, setup the falcon data pointer in Fwsec.
+ // We need mutable access here, so we handle the Option manually.
+ let final_fwsec_image = {
+ let mut second = second_fwsec_image; // Take ownership of the option
+
+ if let (Some(second), Some(first), Some(pci_at)) =
+ (second.as_mut(), first_fwsec_image, pci_at_image)
+ {
+ second
+ .setup_falcon_data(pdev, &pci_at, &first)
+ .inspect_err(|e| {
+ dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
+ })?;
+ } else {
+ dev_err!(
+ pdev.as_ref(),
+ "Missing required images for falcon data setup, skipping\n"
+ );
+ return Err(EINVAL);
+ }
+ second
+ };
+
+ Ok(Vbios {
+ fwsec_image: final_fwsec_image,
+ })
+ }
+
+ pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
+ let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
+ image.fwsec_header(pdev)
+ }
+
+ pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
+ let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
+ image.fwsec_ucode(pdev, image.fwsec_header(pdev)?)
+ }
+
+ pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
+ let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
+ image.fwsec_sigs(pdev, image.fwsec_header(pdev)?)
+ }
+}
+
+/// PCI Data Structure as defined in PCI Firmware Specification
+#[derive(Debug, Clone)]
+#[repr(C)]
+struct PcirStruct {
+ /// PCI Data Structure signature ("PCIR" or "NPDS")
+ pub signature: [u8; 4],
+ /// PCI Vendor ID (e.g., 0x10DE for NVIDIA)
+ pub vendor_id: u16,
+ /// PCI Device ID
+ pub device_id: u16,
+ /// Device List Pointer
+ pub device_list_ptr: u16,
+ /// PCI Data Structure Length
+ pub pci_data_struct_len: u16,
+ /// PCI Data Structure Revision
+ pub pci_data_struct_rev: u8,
+ /// Class code (3 bytes, 0x03 for display controller)
+ pub class_code: [u8; 3],
+ /// Size of this image in 512-byte blocks
+ pub image_len: u16,
+ /// Revision Level of the Vendor's ROM
+ pub vendor_rom_rev: u16,
+ /// ROM image type (0x00 = PC-AT compatible, 0x03 = EFI, 0x70 = NBSI)
+ pub code_type: u8,
+ /// Last image indicator (0x00 = Not last image, 0x80 = Last image)
+ pub last_image: u8,
+ /// Maximum Run-time Image Length (units of 512 bytes)
+ pub max_runtime_image_len: u16,
+}
+
+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 {
+ // Image size is in 512-byte blocks
+ Ok(self.image_len as usize * 512)
+ } else {
+ Err(EINVAL)
+ }
+ }
+}
+
+/// BIOS Information Table (BIT) Header
+/// This is the head of the BIT table, that is used to locate the Falcon data.
+/// The BIT table (with its header) is in the PciAtBiosImage and the falcon data
+/// it is pointing to is in the FwSecBiosImage.
+#[derive(Debug, Clone, Copy)]
+#[expect(dead_code)]
+struct BitHeader {
+ /// 0h: BIT Header Identifier (BMP=0x7FFF/BIT=0xB8FF)
+ pub id: u16,
+ /// 2h: BIT Header Signature ("BIT\0")
+ pub signature: [u8; 4],
+ /// 6h: Binary Coded Decimal Version, ex: 0x0100 is 1.00.
+ pub bcd_version: u16,
+ /// 8h: Size of BIT Header (in bytes)
+ pub header_size: u8,
+ /// 9h: Size of BIT Tokens (in bytes)
+ pub token_size: u8,
+ /// 10h: Number of token entries that follow
+ pub token_entries: u8,
+ /// 11h: BIT Header Checksum
+ pub checksum: u8,
+}
+
+impl BitHeader {
+ fn new(data: &[u8]) -> Result<Self> {
+ if data.len() < 12 {
+ return Err(EINVAL);
+ }
+
+ let mut signature = [0u8; 4];
+ signature.copy_from_slice(&data[2..6]);
+
+ // Check header ID and signature
+ let id = u16::from_le_bytes([data[0], data[1]]);
+ if id != 0xB8FF || &signature != b"BIT\0" {
+ return Err(EINVAL);
+ }
+
+ Ok(BitHeader {
+ id,
+ signature,
+ bcd_version: u16::from_le_bytes([data[6], data[7]]),
+ header_size: data[8],
+ token_size: data[9],
+ token_entries: data[10],
+ checksum: data[11],
+ })
+ }
+}
+
+/// BIT Token Entry: Records in the BIT table followed by the BIT header
+#[derive(Debug, Clone, Copy)]
+#[expect(dead_code)]
+struct BitToken {
+ /// 00h: Token identifier
+ pub id: u8,
+ /// 01h: Version of the token data
+ pub data_version: u8,
+ /// 02h: Size of token data in bytes
+ pub data_size: u16,
+ /// 04h: Offset to the token data
+ pub data_offset: u16,
+}
+
+// Define the token ID for the Falcon data
+pub(in crate::vbios) const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
+
+impl BitToken {
+ /// Find a BIT token entry by BIT ID in a PciAtBiosImage
+ pub(in crate::vbios) fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
+ let header = image.bit_header.as_ref().ok_or(EINVAL)?;
+
+ // Offset to the first token entry
+ let tokens_start = image.bit_offset.ok_or(EINVAL)? + header.header_size as usize;
+
+ for i in 0..header.token_entries as usize {
+ let entry_offset = tokens_start + (i * header.token_size as usize);
+
+ // Make sure we don't go out of bounds
+ if entry_offset + header.token_size as usize > image.base.data.len() {
+ return Err(EINVAL);
+ }
+
+ // Check if this token has the requested ID
+ if image.base.data[entry_offset] == token_id {
+ return Ok(BitToken {
+ id: image.base.data[entry_offset],
+ data_version: image.base.data[entry_offset + 1],
+ data_size: u16::from_le_bytes([
+ image.base.data[entry_offset + 2],
+ image.base.data[entry_offset + 3],
+ ]),
+ data_offset: u16::from_le_bytes([
+ image.base.data[entry_offset + 4],
+ image.base.data[entry_offset + 5],
+ ]),
+ });
+ }
+ }
+
+ // Token not found
+ Err(ENOENT)
+ }
+}
+
+/// PCI ROM Expansion Header as defined in PCI Firmware Specification.
+/// This is header is at the beginning of every image in the set of
+/// images in the ROM. It contains a pointer to the PCI Data Structure
+/// which describes the image.
+/// For "NBSI" images (NoteBook System Information), the ROM
+/// header deviates from the standard and contains an offset to the
+/// NBSI image however we do not yet parse that in this module and keep
+/// it for future reference.
+#[derive(Debug, Clone, Copy)]
+#[expect(dead_code)]
+struct PciRomHeader {
+ /// 00h: Signature (0xAA55)
+ pub signature: u16,
+ /// 02h: Reserved bytes for processor architecture unique data (20 bytes)
+ pub reserved: [u8; 20],
+ /// 16h: NBSI Data Offset (NBSI-specific, offset from header to NBSI image)
+ pub nbsi_data_offset: Option<u16>,
+ /// 18h: Pointer to PCI Data Structure (offset from start of ROM image)
+ pub pci_data_struct_offset: u16,
+ /// 1Ah: Size of block (this is NBSI-specific)
+ pub size_of_block: Option<u32>,
+}
+
+impl PciRomHeader {
+ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+ if data.len() < 26 {
+ // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock
+ return Err(EINVAL);
+ }
+
+ let signature = u16::from_le_bytes([data[0], data[1]]);
+
+ // Check for valid ROM signatures
+ match signature {
+ 0xAA55 | 0xBB77 | 0x4E56 => {}
+ _ => {
+ dev_err!(pdev.as_ref(), "ROM signature unknown {:#x}\n", signature);
+ return Err(EINVAL);
+ }
+ }
+
+ // Read the pointer to the PCI Data Structure at offset 0x18
+ let pci_data_struct_ptr = u16::from_le_bytes([data[24], data[25]]);
+
+ // Try to read optional fields if enough data
+ let mut size_of_block = None;
+ let mut nbsi_data_offset = None;
+
+ if data.len() >= 30 {
+ // Read size_of_block at offset 0x1A
+ size_of_block = Some(
+ (data[29] as u32) << 24
+ | (data[28] as u32) << 16
+ | (data[27] as u32) << 8
+ | (data[26] as u32),
+ );
+ }
+
+ // For NBSI images, try to read the nbsiDataOffset at offset 0x16
+ if data.len() >= 24 {
+ nbsi_data_offset = Some(u16::from_le_bytes([data[22], data[23]]));
+ }
+
+ Ok(PciRomHeader {
+ signature,
+ reserved: [0u8; 20],
+ pci_data_struct_offset: pci_data_struct_ptr,
+ size_of_block,
+ nbsi_data_offset,
+ })
+ }
+}
+
+/// NVIDIA PCI Data Extension Structure. This is similar to the
+/// PCI Data Structure, but is Nvidia-specific and is placed right after
+/// the PCI Data Structure. It contains some fields that are redundant
+/// with the PCI Data Structure, but are needed for traversing the
+/// BIOS images. It is expected to be present in all BIOS images except
+/// for NBSI images.
+#[derive(Debug, Clone)]
+#[expect(dead_code)]
+struct NpdeStruct {
+ /// 00h: Signature ("NPDE")
+ pub signature: [u8; 4],
+ /// 04h: NVIDIA PCI Data Extension Revision
+ pub npci_data_ext_rev: u16,
+ /// 06h: NVIDIA PCI Data Extension Length
+ pub npci_data_ext_len: u16,
+ /// 08h: Sub-image Length (in 512-byte units)
+ pub subimage_len: u16,
+ /// 0Ah: Last image indicator flag
+ pub last_image: u8,
+}
+
+impl NpdeStruct {
+ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+ if data.len() < 11 {
+ dev_err!(pdev.as_ref(), "Not enough data for NpdeStruct\n");
+ return Err(EINVAL);
+ }
+
+ let mut signature = [0u8; 4];
+ signature.copy_from_slice(&data[0..4]);
+
+ // Signature should be "NPDE" (0x4544504E)
+ if &signature != b"NPDE" {
+ dev_err!(
+ pdev.as_ref(),
+ "Invalid signature for NpdeStruct: {:?}\n",
+ signature
+ );
+ return Err(EINVAL);
+ }
+
+ Ok(NpdeStruct {
+ signature,
+ npci_data_ext_rev: u16::from_le_bytes([data[4], data[5]]),
+ npci_data_ext_len: u16::from_le_bytes([data[6], data[7]]),
+ subimage_len: u16::from_le_bytes([data[8], data[9]]),
+ last_image: data[10],
+ })
+ }
+
+ /// 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.subimage_len > 0 {
+ // Image size is in 512-byte blocks
+ Ok(self.subimage_len as usize * 512)
+ } else {
+ Err(EINVAL)
+ }
+ }
+
+ /// 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;
+
+ // Check if we have enough data
+ if npde_start + 11 > data.len() {
+ dev_err!(pdev.as_ref(), "Not enough data for NPDE\n");
+ 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()
+ }
+}
+
+// Use a macro to implement BiosImage enum and methods. This avoids having to
+// repeat each enum type when implementing functions like base() in BiosImage.
+macro_rules! bios_image {
+ (
+ $($variant:ident $class:ident),* $(,)?
+ ) => {
+ // BiosImage enum with variants for each image type
+ enum BiosImage {
+ $($variant($class)),*
+ }
+
+ impl BiosImage {
+ /// Get a reference to the common BIOS image data regardless of type
+ fn base(&self) -> &BiosImageBase {
+ match self {
+ $(Self::$variant(img) => &img.base),*
+ }
+ }
+
+ /// Returns a string representing the type of BIOS image
+ fn image_type_str(&self) -> &'static str {
+ match self {
+ $(Self::$variant(_) => stringify!($variant)),*
+ }
+ }
+ }
+ }
+}
+
+impl BiosImage {
+ /// Check if this is the last image
+ fn is_last(&self) -> bool {
+ let base = self.base();
+
+ // For NBSI images (type == 0x70), return true as they're
+ // considered the last image
+ if matches!(self, Self::Nbsi(_)) {
+ return true;
+ }
+
+ // For other image types, check the NPDE first if available
+ if let Some(ref npde) = base.npde {
+ return npde.is_last();
+ }
+
+ // Otherwise, fall back to checking the PCIR last_image flag
+ base.pcir.is_last()
+ }
+
+ /// Get the image size in bytes
+ fn image_size_bytes(&self) -> Result<usize> {
+ let base = self.base();
+
+ // Prefer NPDE image size if available
+ if let Some(ref npde) = base.npde {
+ return npde.image_size_bytes();
+ }
+
+ // Otherwise, fall back to the PCIR image size
+ base.pcir.image_size_bytes()
+ }
+
+ /// Create a BiosImageBase from a byte slice and convert it to a BiosImage
+ /// which triggers the constructor of the specific BiosImage enum variant.
+ fn new(pdev: &pci::Device, data: &[u8]) -> Result<Self> {
+ let base = BiosImageBase::new(pdev, data)?;
+ let image = base.into_image().inspect_err(|e| {
+ dev_err!(pdev.as_ref(), "Failed to create BiosImage: {:?}\n", e);
+ })?;
+
+ image.image_size_bytes().inspect_err(|_| {
+ dev_err!(
+ pdev.as_ref(),
+ "Invalid image size computed during BiosImage creation\n"
+ )
+ })?;
+
+ Ok(image)
+ }
+}
+
+bios_image! {
+ PciAt PciAtBiosImage, // PCI-AT compatible BIOS image
+ Efi EfiBiosImage, // EFI (Extensible Firmware Interface)
+ Nbsi NbsiBiosImage, // NBSI (Nvidia Bios System Interface)
+ FwSec FwSecBiosImage // FWSEC (Firmware Security)
+}
+
+struct PciAtBiosImage {
+ base: BiosImageBase,
+ bit_header: Option<BitHeader>,
+ bit_offset: Option<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.
+}
+
+pub(crate) struct FwSecBiosImage {
+ base: BiosImageBase,
+ // FWSEC-specific fields
+ // 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>,
+}
+
+// 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::FwSec(FwSecBiosImage {
+ 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]) -> Option<usize> {
+ haystack
+ .windows(needle.len())
+ .position(|window| window == needle)
+ }
+
+ /// 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);
+ if bit_offset.is_none() {
+ return Err(EINVAL);
+ }
+
+ let bit_header = BitHeader::new(&data[bit_offset.ok_or(EINVAL)?..])?;
+ Ok((bit_header, bit_offset.ok_or(EINVAL)?))
+ }
+
+ /// 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)
+ }
+}
+
+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: Some(bit_header),
+ bit_offset: Some(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(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 {
+ return Err(EINVAL);
+ }
+
+ // Create a copy of only the table data
+ let mut table_data = KVec::new();
+
+ // "last_entry_bytes" is a debugging aid.
+ let mut last_entry_bytes: Option<KVec<u8>> = if cfg!(debug_assertions) {
+ Some(KVec::new())
+ } else {
+ None
+ };
+
+ for &byte in &data[header_len..required_bytes] {
+ table_data.push(byte, GFP_KERNEL)?;
+
+ if cfg!(debug_assertions) {
+ // Debugging (dumps the table data to dmesg):
+ if let Some(ref mut last_entry_bytes) = last_entry_bytes {
+ last_entry_bytes.push(byte, GFP_KERNEL)?;
+
+ let last_entry_bytes_len = last_entry_bytes.len();
+ if last_entry_bytes_len == entry_len {
+ pr_info!("Last entry bytes: {:02x?}\n", &last_entry_bytes[..]);
+ *last_entry_bytes = KVec::new();
+ }
+ }
+ }
+ }
+
+ 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 FwSecBiosImage {
+ fn setup_falcon_data(
+ &mut self,
+ pdev: &pci::Device,
+ pci_at_image: &PciAtBiosImage,
+ first_fwsec_image: &FwSecBiosImage,
+ ) -> Result<()> {
+ let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
+
+ // 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.
+ offset -= first_fwsec_image.base.data.len();
+
+ self.falcon_data_offset = Some(offset);
+
+ // The PmuLookupTable starts at the offset of the falcon data pointer
+ self.pmu_lookup_table = Some(PmuLookupTable::new(&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();
+ ucode_offset -= first_fwsec_image.base.data.len();
+ self.falcon_ucode_offset = Some(ucode_offset);
+ if cfg!(debug_assertions) {
+ // Print the v3_desc header for debugging
+ let v3_desc = self.fwsec_header(pdev.as_ref())?;
+ pr_info!("PmuLookupTableEntry v3_desc: {:#?}\n", v3_desc);
+ }
+ }
+ Err(e) => {
+ dev_err!(
+ pdev.as_ref(),
+ "PmuLookupTableEntry not found, error: {:?}\n",
+ e
+ );
+ }
+ }
+ Ok(())
+ }
+
+ /// TODO: These were borrowed from the old code for integrating this module
+ /// with the outside world. They should be cleaned up and integrated properly.
+ ///
+ /// 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.ok_or(EINVAL)?;
+
+ // 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");
+ 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.`
+ Ok(unsafe {
+ &*(self.base.data.as_ptr().add(falcon_ucode_offset) as *const FalconUCodeDescV3)
+ })
+ }
+ /// Get the ucode data as a byte slice
+ fn fwsec_ucode(&self, dev: &device::Device, v3_desc: &FalconUCodeDescV3) -> Result<&[u8]> {
+ let falcon_ucode_offset = self.falcon_ucode_offset.ok_or(EINVAL)?;
+
+ // The ucode data follows the descriptor
+ let ucode_data_offset = falcon_ucode_offset + v3_desc.size();
+ let size = (v3_desc.imem_load_size + v3_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, v3_desc: &FalconUCodeDescV3) -> Result<&[u8]> {
+ const SIG_SIZE: usize = 96 * 4;
+
+ let falcon_ucode_offset = self.falcon_ucode_offset.ok_or(EINVAL)?;
+
+ // The signatures data follows the descriptor
+ let sigs_data_offset = falcon_ucode_offset + core::mem::size_of::<FalconUCodeDescV3>();
+ let size = v3_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])
+ }
+}
--
2.49.0On Wed, 2025-05-07 at 22:52 +0900, Alexandre Courbot wrote:
> +impl FwSecBiosImage {
> + fn setup_falcon_data(
> + &mut self,
> + pdev: &pci::Device,
> + pci_at_image: &PciAtBiosImage,
> + first_fwsec_image: &FwSecBiosImage,
> + ) -> Result<()> {
> + let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
> +
> + // 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.
> + offset -= first_fwsec_image.base.data.len();
> +
> + self.falcon_data_offset = Some(offset);
> +
> + // The PmuLookupTable starts at the offset of the falcon data pointer
> + self.pmu_lookup_table = Some(PmuLookupTable::new(&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();
> + ucode_offset -= first_fwsec_image.base.data.len();
> + self.falcon_ucode_offset = Some(ucode_offset);
> + if cfg!(debug_assertions) {
> + // Print the v3_desc header for debugging
> + let v3_desc = self.fwsec_header(pdev.as_ref())?;
> + pr_info!("PmuLookupTableEntry v3_desc: {:#?}\n", v3_desc);
> + }
> + }
> + Err(e) => {
> + dev_err!(
> + pdev.as_ref(),
> + "PmuLookupTableEntry not found, error: {:?}\n",
> + e
> + );
Shouldn't you return an error here?
If not, then maybe this should probably be dev_warn.
On 5/16/2025 4:38 PM, Timur Tabi wrote:
> n Wed, 2025-05-07 at 22:52 +0900, Alexandre Courbot wrote:
>> +impl FwSecBiosImage {
>> + fn setup_falcon_data(
>> + &mut self,
>> + pdev: &pci::Device,
>> + pci_at_image: &PciAtBiosImage,
>> + first_fwsec_image: &FwSecBiosImage,
>> + ) -> Result<()> {
>> + let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
>> +
>> + // 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.
>> + offset -= first_fwsec_image.base.data.len();
>> +
>> + self.falcon_data_offset = Some(offset);
>> +
>> + // The PmuLookupTable starts at the offset of the falcon data pointer
>> + self.pmu_lookup_table = Some(PmuLookupTable::new(&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();
>> + ucode_offset -= first_fwsec_image.base.data.len();
>> + self.falcon_ucode_offset = Some(ucode_offset);
>> + if cfg!(debug_assertions) {
>> + // Print the v3_desc header for debugging
>> + let v3_desc = self.fwsec_header(pdev.as_ref())?;
>> + pr_info!("PmuLookupTableEntry v3_desc: {:#?}\n", v3_desc);
>> + }
>> + }
>> + Err(e) => {
>> + dev_err!(
>> + pdev.as_ref(),
>> + "PmuLookupTableEntry not found, error: {:?}\n",
>> + e
>> + );
> Shouldn't you return an error here?
>
> If not, then maybe this should probably be dev_warn.
Good catch, fixed! Thanks,
- Joel
On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
> +/// PCI Data Structure as defined in PCI Firmware Specification
> +#[derive(Debug, Clone)]
> +#[repr(C)]
> +struct PcirStruct {
> + /// PCI Data Structure signature ("PCIR" or "NPDS")
> + pub signature: [u8; 4],
> + /// PCI Vendor ID (e.g., 0x10DE for NVIDIA)
> + pub vendor_id: u16,
> + /// PCI Device ID
> + pub device_id: u16,
> + /// Device List Pointer
> + pub device_list_ptr: u16,
> + /// PCI Data Structure Length
> + pub pci_data_struct_len: u16,
> + /// PCI Data Structure Revision
> + pub pci_data_struct_rev: u8,
> + /// Class code (3 bytes, 0x03 for display controller)
> + pub class_code: [u8; 3],
> + /// Size of this image in 512-byte blocks
> + pub image_len: u16,
> + /// Revision Level of the Vendor's ROM
> + pub vendor_rom_rev: u16,
> + /// ROM image type (0x00 = PC-AT compatible, 0x03 = EFI, 0x70 = NBSI)
> + pub code_type: u8,
> + /// Last image indicator (0x00 = Not last image, 0x80 = Last image)
> + pub last_image: u8,
> + /// Maximum Run-time Image Length (units of 512 bytes)
> + pub max_runtime_image_len: u16,
> +}
Here and in a couple more cases below, please don't use pub for fields of
private structures.
> +
> +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]]),
> + })
Quite some of those fields seem unused, do we still want to have them? Same for
other structures below.
> + }
> +
> + /// 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 {
> + // Image size is in 512-byte blocks
> + Ok(self.image_len as usize * 512)
> + } else {
> + Err(EINVAL)
> + }
> + }
> +}
> +
> +/// BIOS Information Table (BIT) Header
> +/// This is the head of the BIT table, that is used to locate the Falcon data.
> +/// The BIT table (with its header) is in the PciAtBiosImage and the falcon data
> +/// it is pointing to is in the FwSecBiosImage.
> +#[derive(Debug, Clone, Copy)]
> +#[expect(dead_code)]
> +struct BitHeader {
> + /// 0h: BIT Header Identifier (BMP=0x7FFF/BIT=0xB8FF)
> + pub id: u16,
> + /// 2h: BIT Header Signature ("BIT\0")
> + pub signature: [u8; 4],
> + /// 6h: Binary Coded Decimal Version, ex: 0x0100 is 1.00.
> + pub bcd_version: u16,
> + /// 8h: Size of BIT Header (in bytes)
> + pub header_size: u8,
> + /// 9h: Size of BIT Tokens (in bytes)
> + pub token_size: u8,
> + /// 10h: Number of token entries that follow
> + pub token_entries: u8,
> + /// 11h: BIT Header Checksum
> + pub checksum: u8,
> +}
> +
> +impl BitHeader {
> + fn new(data: &[u8]) -> Result<Self> {
> + if data.len() < 12 {
> + return Err(EINVAL);
> + }
> +
> + let mut signature = [0u8; 4];
> + signature.copy_from_slice(&data[2..6]);
> +
> + // Check header ID and signature
> + let id = u16::from_le_bytes([data[0], data[1]]);
> + if id != 0xB8FF || &signature != b"BIT\0" {
> + return Err(EINVAL);
> + }
> +
> + Ok(BitHeader {
> + id,
> + signature,
> + bcd_version: u16::from_le_bytes([data[6], data[7]]),
> + header_size: data[8],
> + token_size: data[9],
> + token_entries: data[10],
> + checksum: data[11],
> + })
> + }
> +}
> +
> +/// BIT Token Entry: Records in the BIT table followed by the BIT header
> +#[derive(Debug, Clone, Copy)]
> +#[expect(dead_code)]
> +struct BitToken {
> + /// 00h: Token identifier
> + pub id: u8,
> + /// 01h: Version of the token data
> + pub data_version: u8,
> + /// 02h: Size of token data in bytes
> + pub data_size: u16,
> + /// 04h: Offset to the token data
> + pub data_offset: u16,
> +}
> +
> +// Define the token ID for the Falcon data
> +pub(in crate::vbios) const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
This can just be private.
> +
> +impl BitToken {
> + /// Find a BIT token entry by BIT ID in a PciAtBiosImage
> + pub(in crate::vbios) fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
Same here.
<snip>
> +struct PciAtBiosImage {
> + base: BiosImageBase,
> + bit_header: Option<BitHeader>,
> + bit_offset: Option<usize>,
Why are those Options? AFAICS, this structure is only ever created from
impl TryFrom<BiosImageBase> for PciAtBiosImage
and there you fail if you can't find the bit header anyways.
Also BitToken::from_id fails if bit_header == None, and it doesn't seem to be
used anywhere else.
I think we should remove the Option wrapper for both.
<snip>
> +/// 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(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 {
> + return Err(EINVAL);
> + }
> +
> + // Create a copy of only the table data
> + let mut table_data = KVec::new();
> +
> + // "last_entry_bytes" is a debugging aid.
> + let mut last_entry_bytes: Option<KVec<u8>> = if cfg!(debug_assertions) {
> + Some(KVec::new())
> + } else {
> + None
> + };
> +
> + for &byte in &data[header_len..required_bytes] {
> + table_data.push(byte, GFP_KERNEL)?;
This should just be
table_data.extend_from_slice(&data[header_len..required_bytes], GFP_KERNEL)?;
so you don't need the loop and potentially lots of re-allocations.
Subsequently you can implement the debugging stuff as
if cfg!(debug_assertions) {
let mut last_entry_bytes = KVec::new();
for &byte in &data[header_len..required_bytes] {
// Debugging (dumps the table data to dmesg):
last_entry_bytes.push(byte, GFP_KERNEL)?;
let last_entry_bytes_len = last_entry_bytes.len();
if last_entry_bytes_len == entry_len {
pr_info!("Last entry bytes: {:02x?}\n", &last_entry_bytes[..]);
last_entry_bytes = KVec::new();
}
}
}
In general, I feel like this patch utilizes the Option type way too much and
often without actual need. Can you please also double check?
> +
> + if cfg!(debug_assertions) {
> + // Debugging (dumps the table data to dmesg):
> + if let Some(ref mut last_entry_bytes) = last_entry_bytes {
> + last_entry_bytes.push(byte, GFP_KERNEL)?;
> +
> + let last_entry_bytes_len = last_entry_bytes.len();
> + if last_entry_bytes_len == entry_len {
> + pr_info!("Last entry bytes: {:02x?}\n", &last_entry_bytes[..]);
Please use dev_dbg!().
> + *last_entry_bytes = KVec::new();
> + }
> + }
> + }
> + }
> +
> + 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 FwSecBiosImage {
> + fn setup_falcon_data(
> + &mut self,
> + pdev: &pci::Device,
> + pci_at_image: &PciAtBiosImage,
> + first_fwsec_image: &FwSecBiosImage,
> + ) -> Result<()> {
Just Result will do.
> + let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
> +
> + // 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.
> + offset -= first_fwsec_image.base.data.len();
> +
> + self.falcon_data_offset = Some(offset);
> +
> + // The PmuLookupTable starts at the offset of the falcon data pointer
> + self.pmu_lookup_table = Some(PmuLookupTable::new(&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();
> + ucode_offset -= first_fwsec_image.base.data.len();
> + self.falcon_ucode_offset = Some(ucode_offset);
> + if cfg!(debug_assertions) {
> + // Print the v3_desc header for debugging
> + let v3_desc = self.fwsec_header(pdev.as_ref())?;
> + pr_info!("PmuLookupTableEntry v3_desc: {:#?}\n", v3_desc);
> + }
> + }
> + Err(e) => {
> + dev_err!(
> + pdev.as_ref(),
> + "PmuLookupTableEntry not found, error: {:?}\n",
> + e
> + );
> + }
> + }
> + Ok(())
> + }
> +
> + /// TODO: These were borrowed from the old code for integrating this module
> + /// with the outside world. They should be cleaned up and integrated properly.
Okay, won't review for now then. :)
Hi Danilo,
On 5/14/2025 12:23 PM, Danilo Krummrich wrote:
> I feel like this patch utilizes the Option type way too much and
> often without actual need. Can you please also double check?
>
I found one other instance (vbios.fwsec_image). Other than that, all others are
required AFAICS.
>> +
>> + if cfg!(debug_assertions) {
>> + // Debugging (dumps the table data to dmesg):
>> + if let Some(ref mut last_entry_bytes) = last_entry_bytes {
>> + last_entry_bytes.push(byte, GFP_KERNEL)?;
>> +
>> + let last_entry_bytes_len = last_entry_bytes.len();
>> + if last_entry_bytes_len == entry_len {
>> + pr_info!("Last entry bytes: {:02x?}\n", &last_entry_bytes[..]);
>
> Please use dev_dbg!().
>
This required passing down the pdev here, but did that, thanks.
>> + *last_entry_bytes = KVec::new();
>> + }
>> + }
>> + }
>> + }
>> +
>> + 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 FwSecBiosImage {
>> + fn setup_falcon_data(
>> + &mut self,
>> + pdev: &pci::Device,
>> + pci_at_image: &PciAtBiosImage,
>> + first_fwsec_image: &FwSecBiosImage,
>> + ) -> Result<()> {
>
> Just Result will do.
>
Fixed.
>> + let mut offset = pci_at_image.falcon_data_ptr(pdev)? as usize;
>> +
>> + // 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.
>> + offset -= first_fwsec_image.base.data.len();
>> +
>> + self.falcon_data_offset = Some(offset);
>> +
>> + // The PmuLookupTable starts at the offset of the falcon data pointer
>> + self.pmu_lookup_table = Some(PmuLookupTable::new(&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();
>> + ucode_offset -= first_fwsec_image.base.data.len();
>> + self.falcon_ucode_offset = Some(ucode_offset);
>> + if cfg!(debug_assertions) {
>> + // Print the v3_desc header for debugging
>> + let v3_desc = self.fwsec_header(pdev.as_ref())?;
>> + pr_info!("PmuLookupTableEntry v3_desc: {:#?}\n", v3_desc);
>> + }
>> + }
>> + Err(e) => {
>> + dev_err!(
>> + pdev.as_ref(),
>> + "PmuLookupTableEntry not found, error: {:?}\n",
>> + e
>> + );
>> + }
>> + }
>> + Ok(())
>> + }
>> +
>> + /// TODO: These were borrowed from the old code for integrating this module
>> + /// with the outside world. They should be cleaned up and integrated properly.
>
> Okay, won't review for now then. 🙂
In the next revision, we are removing this TODO and can continue review. :)
thanks,
- Joel
Hi Danilo,
On 5/14/2025 12:23 PM, Danilo Krummrich wrote:
> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
>> +/// PCI Data Structure as defined in PCI Firmware Specification
>> +#[derive(Debug, Clone)]
>> +#[repr(C)]
>> +struct PcirStruct {
>> + /// PCI Data Structure signature ("PCIR" or "NPDS")
>> + pub signature: [u8; 4],
>> + /// PCI Vendor ID (e.g., 0x10DE for NVIDIA)
>> + pub vendor_id: u16,
>> + /// PCI Device ID
>> + pub device_id: u16,
>> + /// Device List Pointer
>> + pub device_list_ptr: u16,
>> + /// PCI Data Structure Length
>> + pub pci_data_struct_len: u16,
>> + /// PCI Data Structure Revision
>> + pub pci_data_struct_rev: u8,
>> + /// Class code (3 bytes, 0x03 for display controller)
>> + pub class_code: [u8; 3],
>> + /// Size of this image in 512-byte blocks
>> + pub image_len: u16,
>> + /// Revision Level of the Vendor's ROM
>> + pub vendor_rom_rev: u16,
>> + /// ROM image type (0x00 = PC-AT compatible, 0x03 = EFI, 0x70 = NBSI)
>> + pub code_type: u8,
>> + /// Last image indicator (0x00 = Not last image, 0x80 = Last image)
>> + pub last_image: u8,
>> + /// Maximum Run-time Image Length (units of 512 bytes)
>> + pub max_runtime_image_len: u16,
>> +}
>
> Here and in a couple more cases below, please don't use pub for fields of
> private structures.
Fixed thanks.
>> +
>> +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]]),
>> + })
>
> Quite some of those fields seem unused, do we still want to have them? Same for
> other structures below.
I think we discussed this in the previous posting as well. As such, I am not
keen on removing unused fields of structures part of 'standard' specifications
since I only see drawbacks of doing so:
1. Obfuscation
2. Replacement of the fields with some kind of padding so that size_of() works.
3. Goes in the opposite direction of documentation and transparency in how the
structures work.
4. Partially filling structures.
>> +
>> + /// 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 {
>> + // Image size is in 512-byte blocks
>> + Ok(self.image_len as usize * 512)
>> + } else {
>> + Err(EINVAL)
>> + }
>> + }
>> +}
>> +
>> +/// BIOS Information Table (BIT) Header
>> +/// This is the head of the BIT table, that is used to locate the Falcon data.
>> +/// The BIT table (with its header) is in the PciAtBiosImage and the falcon data
>> +/// it is pointing to is in the FwSecBiosImage.
>> +#[derive(Debug, Clone, Copy)]
>> +#[expect(dead_code)]
>> +struct BitHeader {
>> + /// 0h: BIT Header Identifier (BMP=0x7FFF/BIT=0xB8FF)
>> + pub id: u16,
>> + /// 2h: BIT Header Signature ("BIT\0")
>> + pub signature: [u8; 4],
>> + /// 6h: Binary Coded Decimal Version, ex: 0x0100 is 1.00.
>> + pub bcd_version: u16,
>> + /// 8h: Size of BIT Header (in bytes)
>> + pub header_size: u8,
>> + /// 9h: Size of BIT Tokens (in bytes)
>> + pub token_size: u8,
>> + /// 10h: Number of token entries that follow
>> + pub token_entries: u8,
>> + /// 11h: BIT Header Checksum
>> + pub checksum: u8,
>> +}
>> +
>> +impl BitHeader {
>> + fn new(data: &[u8]) -> Result<Self> {
>> + if data.len() < 12 {
>> + return Err(EINVAL);
>> + }
>> +
>> + let mut signature = [0u8; 4];
>> + signature.copy_from_slice(&data[2..6]);
>> +
>> + // Check header ID and signature
>> + let id = u16::from_le_bytes([data[0], data[1]]);
>> + if id != 0xB8FF || &signature != b"BIT\0" {
>> + return Err(EINVAL);
>> + }
>> +
>> + Ok(BitHeader {
>> + id,
>> + signature,
>> + bcd_version: u16::from_le_bytes([data[6], data[7]]),
>> + header_size: data[8],
>> + token_size: data[9],
>> + token_entries: data[10],
>> + checksum: data[11],
>> + })
>> + }
>> +}
>> +
>> +/// BIT Token Entry: Records in the BIT table followed by the BIT header
>> +#[derive(Debug, Clone, Copy)]
>> +#[expect(dead_code)]
>> +struct BitToken {
>> + /// 00h: Token identifier
>> + pub id: u8,
>> + /// 01h: Version of the token data
>> + pub data_version: u8,
>> + /// 02h: Size of token data in bytes
>> + pub data_size: u16,
>> + /// 04h: Offset to the token data
>> + pub data_offset: u16,
>> +}
>> +
>> +// Define the token ID for the Falcon data
>> +pub(in crate::vbios) const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
>
> This can just be private.
Yep, fixed.
>> +
>> +impl BitToken {
>> + /// Find a BIT token entry by BIT ID in a PciAtBiosImage
>> + pub(in crate::vbios) fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
>
> Same here.
Fixed.
> <snip>
>
>> +struct PciAtBiosImage {
>> + base: BiosImageBase,
>> + bit_header: Option<BitHeader>,
>> + bit_offset: Option<usize>,
>
> Why are those Options? AFAICS, this structure is only ever created from
>
> impl TryFrom<BiosImageBase> for PciAtBiosImage
>
> and there you fail if you can't find the bit header anyways.
>
> Also BitToken::from_id fails if bit_header == None, and it doesn't seem to be
> used anywhere else.
>
> I think we should remove the Option wrapper for both.
Yes, thanks. That does simplify the code, I made the change and it works.
>
>> +/// 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(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 {
>> + return Err(EINVAL);
>> + }
>> +
>> + // Create a copy of only the table data
>> + let mut table_data = KVec::new();
>> +
>> + // "last_entry_bytes" is a debugging aid.
>> + let mut last_entry_bytes: Option<KVec<u8>> = if cfg!(debug_assertions) {
>> + Some(KVec::new())
>> + } else {
>> + None
>> + };
>> +
>> + for &byte in &data[header_len..required_bytes] {
>> + table_data.push(byte, GFP_KERNEL)?;
>
> This should just be
>
> table_data.extend_from_slice(&data[header_len..required_bytes], GFP_KERNEL)?;
>
> so you don't need the loop and potentially lots of re-allocations.
>
> Subsequently you can implement the debugging stuff as
>
> if cfg!(debug_assertions) {
> let mut last_entry_bytes = KVec::new();
>
> for &byte in &data[header_len..required_bytes] {
> // Debugging (dumps the table data to dmesg):
> last_entry_bytes.push(byte, GFP_KERNEL)?;
>
> let last_entry_bytes_len = last_entry_bytes.len();
> if last_entry_bytes_len == entry_len {
> pr_info!("Last entry bytes: {:02x?}\n", &last_entry_bytes[..]);
> last_entry_bytes = KVec::new();
> }
> }
> }
Ok, that's better, I took the opportunity to replace this code with:
(Sorry for wrapping)
// Create a copy of only the table data
let data_entries = &data[header_len..required_bytes];
let table_data = {
let mut ret = KVec::new();
ret.extend_from_slice(&data_entries, GFP_KERNEL)?;
ret
};
// Debug logging of entries (dumps the table data to dmesg)
if cfg!(debug_assertions) {
for i in 0..entry_count {
pr_info!("PMU entry: {:02x?}\n", &data_entries[i * entry_len..(i
+ 1) * entry_len]);
}
}
> In general, I feel like this patch utilizes the Option type way too much and
> often without actual need. Can you please also double check?
Yeah, sorry, I'm somewhat new to rust. :-D. I am going through all my Options now.
I will continue addressing the rest of the comments and those in the other email
and will reply soon. Thanks!
- Joel
On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
> From: Joel Fernandes <joelagnelf@nvidia.com>
>
> Add support for navigating and setting up vBIOS ucode data required for
> GSP to boot. The main data extracted from the vBIOS is the FWSEC-FRTS
> firmware which runs on the GSP processor. This firmware runs in high
> secure mode, and sets up the WPR2 (Write protected region) before the
> Booter runs on the SEC2 processor.
>
> Also add log messages to show the BIOS images.
>
> [102141.013287] NovaCore: Found BIOS image at offset 0x0, size: 0xfe00, type: PciAt
> [102141.080692] NovaCore: Found BIOS image at offset 0xfe00, size: 0x14800, type: Efi
> [102141.098443] NovaCore: Found BIOS image at offset 0x24600, size: 0x5600, type: FwSec
> [102141.415095] NovaCore: Found BIOS image at offset 0x29c00, size: 0x60800, type: FwSec
>
> Tested on my Ampere GA102 and boot is successful.
>
> [applied changes by Alex Courbot for fwsec signatures]
> [applied feedback from Alex Courbot and Timur Tabi]
> [applied changes related to code reorg, prints etc from Danilo Krummrich]
> [acourbot@nvidia.com: fix clippy warnings]
> [acourbot@nvidia.com: remove now-unneeded Devres acquisition]
> [acourbot@nvidia.com: fix read_more to read `len` bytes, not u32s]
>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Shirish Baskaran <sbaskaran@nvidia.com>
> Cc: Alistair Popple <apopple@nvidia.com>
> Cc: Timur Tabi <ttabi@nvidia.com>
> Cc: Ben Skeggs <bskeggs@nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/gpu/nova-core/firmware.rs | 2 -
> drivers/gpu/nova-core/gpu.rs | 3 +
> drivers/gpu/nova-core/nova_core.rs | 1 +
> drivers/gpu/nova-core/vbios.rs | 1147 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 1151 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 1eb216307cd01d975b3d5beda1dc516f34b4b3f2..960982174d834c7c66a47ecfb3a15bf47116b2c5 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -80,8 +80,6 @@ pub(crate) struct FalconUCodeDescV3 {
> _reserved: u16,
> }
>
> -// To be removed once that code is used.
> -#[expect(dead_code)]
> impl FalconUCodeDescV3 {
> pub(crate) fn size(&self) -> usize {
> ((self.hdr & 0xffff0000) >> 16) as usize
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index ece13594fba687f3f714e255b5436e72d80dece3..4bf7f72247e5320935a517270b5a0e1ec2becfec 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -9,6 +9,7 @@
> use crate::firmware::Firmware;
> use crate::regs;
> use crate::util;
> +use crate::vbios::Vbios;
> use core::fmt;
>
> macro_rules! define_chipset {
> @@ -238,6 +239,8 @@ pub(crate) fn new(
>
> let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
>
> + let _bios = Vbios::new(pdev, bar)?;
Please add a comment why, even though unused, it is important to create this
instance.
Also, please use `_` if it's not intended to ever be used.
> +
> Ok(pin_init!(Self {
> spec,
> bar: devres_bar,
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index 8342482a1aa16da2e69f7d99143c1549a82c969e..ff6d0b40c18f36af4c7e2d5c839fdf77dba23321 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -10,6 +10,7 @@
> mod gpu;
> mod regs;
> mod util;
> +mod vbios;
>
> kernel::module_pci_driver! {
> type: driver::NovaCore,
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..cd55d8dbf8e12d532f776d7544c7e5f2a865d6f8
> --- /dev/null
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -0,0 +1,1147 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! VBIOS extraction and parsing.
> +
> +// To be removed when all code is used.
> +#![expect(dead_code)]
> +
> +use crate::driver::Bar0;
> +use crate::firmware::FalconUCodeDescV3;
> +use core::convert::TryFrom;
> +use kernel::device;
> +use kernel::error::Result;
> +use kernel::num::NumAlign;
> +use kernel::pci;
> +use kernel::prelude::*;
> +
> +/// The offset of the VBIOS ROM in the BAR0 space.
> +const ROM_OFFSET: usize = 0x300000;
> +/// The maximum length of the VBIOS ROM to scan into.
> +const BIOS_MAX_SCAN_LEN: usize = 0x100000;
> +/// The size to read ahead when parsing initial BIOS image headers.
> +const BIOS_READ_AHEAD_SIZE: usize = 1024;
> +/// The bit in the last image indicator byte for the PCI Data Structure that
> +/// indicates the last image. Bit 0-6 are reserved, bit 7 is last image bit.
> +const LAST_IMAGE_BIT_MASK: u8 = 0x80;
> +
> +// PMU lookup table entry types. Used to locate PMU table entries
> +// in the Fwsec image, corresponding to falcon ucodes.
> +#[expect(dead_code)]
> +const FALCON_UCODE_ENTRY_APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
> +#[expect(dead_code)]
> +const FALCON_UCODE_ENTRY_APPID_FWSEC_DBG: u8 = 0x45;
> +const FALCON_UCODE_ENTRY_APPID_FWSEC_PROD: u8 = 0x85;
> +
> +/// Vbios Reader for constructing the VBIOS data
> +struct VbiosIterator<'a> {
> + pdev: &'a pci::Device,
> + bar0: &'a Bar0,
> + // VBIOS data vector: As BIOS images are scanned, they are added to this vector
> + // for reference or copying into other data structures. It is the entire
> + // scanned contents of the VBIOS which progressively extends. It is used
> + // so that we do not re-read any contents that are already read as we use
> + // the cumulative length read so far, and re-read any gaps as we extend
> + // the length.
> + data: KVec<u8>,
> + current_offset: usize, // Current offset for iterator
> + last_found: bool, // Whether the last image has been found
> +}
> +
> +impl<'a> VbiosIterator<'a> {
> + fn new(pdev: &'a pci::Device, bar0: &'a Bar0) -> Result<Self> {
> + Ok(Self {
> + pdev,
> + bar0,
> + data: KVec::new(),
> + current_offset: 0,
> + last_found: false,
> + })
> + }
> +
> + /// Read bytes from the ROM at the current end of the data vector
> + fn read_more(&mut self, len: usize) -> Result {
> + let current_len = self.data.len();
> + let start = ROM_OFFSET + current_len;
> +
> + // Ensure length is a multiple of 4 for 32-bit reads
> + if len % core::mem::size_of::<u32>() != 0 {
> + dev_err!(
> + self.pdev.as_ref(),
> + "VBIOS read length {} is not a multiple of 4\n",
> + len
> + );
> + return Err(EINVAL);
> + }
> +
> + self.data.reserve(len, GFP_KERNEL)?;
> + // Read ROM data bytes and push directly to vector
> + for i in (0..len).step_by(core::mem::size_of::<u32>()) {
> + // Read 32-bit word from the VBIOS ROM
> + let word = self.bar0.try_read32(start + i)?;
> +
> + // Convert the u32 to a 4 byte array and push each byte
> + word.to_ne_bytes()
> + .iter()
> + .try_for_each(|&b| self.data.push(b, GFP_KERNEL))?;
> + }
> +
> + Ok(())
> + }
> +
> + /// Read bytes at a specific offset, filling any gap
> + fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
> + if offset > BIOS_MAX_SCAN_LEN {
> + dev_err!(self.pdev.as_ref(), "Error: exceeded BIOS scan limit.\n");
> + return Err(EINVAL);
> + }
> +
> + // If offset is beyond current data size, fill the gap first
> + let current_len = self.data.len();
> + let gap_bytes = offset.saturating_sub(current_len);
> +
> + // Now read the requested bytes at the offset
> + self.read_more(gap_bytes + len)
> + }
> +
> + /// Read a BIOS image at a specific offset and create a BiosImage from it.
> + /// self.data is extended as needed and a new BiosImage is returned.
> + /// @context is a string describing the operation for error reporting
> + fn read_bios_image_at_offset(
> + &mut self,
> + offset: usize,
> + len: usize,
> + context: &str,
> + ) -> Result<BiosImage> {
> + let data_len = self.data.len();
> + if offset + len > data_len {
> + self.read_more_at_offset(offset, len).inspect_err(|e| {
> + dev_err!(
> + self.pdev.as_ref(),
> + "Failed to read more at offset {:#x}: {:?}\n",
> + offset,
> + e
> + )
> + })?;
> + }
> +
> + BiosImage::new(self.pdev, &self.data[offset..offset + len]).inspect_err(|err| {
> + dev_err!(
> + self.pdev.as_ref(),
> + "Failed to {} at offset {:#x}: {:?}\n",
> + context,
> + offset,
> + err
> + )
> + })
> + }
> +}
> +
> +impl<'a> Iterator for VbiosIterator<'a> {
> + type Item = Result<BiosImage>;
> +
> + /// Iterate over all VBIOS images until the last image is detected or offset
> + /// exceeds scan limit.
> + fn next(&mut self) -> Option<Self::Item> {
> + if self.last_found {
> + return None;
> + }
> +
> + if self.current_offset > BIOS_MAX_SCAN_LEN {
> + dev_err!(
> + self.pdev.as_ref(),
> + "Error: exceeded BIOS scan limit, stopping scan\n"
> + );
> + return None;
> + }
> +
> + // Parse image headers first to get image size
> + let image_size = match self
> + .read_bios_image_at_offset(
> + self.current_offset,
> + BIOS_READ_AHEAD_SIZE,
> + "parse initial BIOS image headers",
> + )
> + .and_then(|image| image.image_size_bytes())
> + {
> + Ok(size) => size,
> + Err(e) => return Some(Err(e)),
> + };
> +
> + // Now create a new BiosImage with the full image data
> + let full_image = match self.read_bios_image_at_offset(
> + self.current_offset,
> + image_size,
> + "parse full BIOS image",
> + ) {
> + Ok(image) => image,
> + Err(e) => return Some(Err(e)),
> + };
> +
> + self.last_found = full_image.is_last();
> +
> + // Advance to next image (aligned to 512 bytes)
> + self.current_offset += image_size;
> + self.current_offset = self.current_offset.align_up(512);
> +
> + Some(Ok(full_image))
> + }
> +}
> +
> +pub(crate) struct Vbios {
> + pub fwsec_image: Option<FwSecBiosImage>,
Please use pub(crate) instead or provide an accessor.
Also, this shouldn't be an Option, see below comment in Vbios::new().
> +}
> +
> +impl Vbios {
> + /// Probe for VBIOS extraction
> + /// Once the VBIOS object is built, bar0 is not read for vbios purposes anymore.
> + pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
> + // Images to extract from iteration
> + let mut pci_at_image: Option<PciAtBiosImage> = None;
> + let mut first_fwsec_image: Option<FwSecBiosImage> = None;
> + let mut second_fwsec_image: Option<FwSecBiosImage> = None;
> +
> + // Parse all VBIOS images in the ROM
> + for image_result in VbiosIterator::new(pdev, bar0)? {
> + let full_image = image_result?;
> +
> + dev_info!(
Let's use dev_dbg!() instaed.
> + pdev.as_ref(),
> + "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
> + full_image.image_size_bytes()?,
> + full_image.image_type_str(),
> + full_image.is_last()
> + );
> +
> + // Get references to images we will need after the loop, in order to
> + // setup the falcon data offset.
> + match full_image {
> + BiosImage::PciAt(image) => {
> + pci_at_image = Some(image);
> + }
> + BiosImage::FwSec(image) => {
> + if first_fwsec_image.is_none() {
> + first_fwsec_image = Some(image);
> + } else {
> + second_fwsec_image = Some(image);
> + }
> + }
> + // For now we don't need to handle these
> + BiosImage::Efi(_image) => {}
> + BiosImage::Nbsi(_image) => {}
> + }
> + }
> +
> + // Using all the images, setup the falcon data pointer in Fwsec.
> + // We need mutable access here, so we handle the Option manually.
> + let final_fwsec_image = {
> + let mut second = second_fwsec_image; // Take ownership of the option
> +
> + if let (Some(second), Some(first), Some(pci_at)) =
> + (second.as_mut(), first_fwsec_image, pci_at_image)
> + {
> + second
> + .setup_falcon_data(pdev, &pci_at, &first)
> + .inspect_err(|e| {
> + dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
> + })?;
> + } else {
> + dev_err!(
> + pdev.as_ref(),
> + "Missing required images for falcon data setup, skipping\n"
> + );
> + return Err(EINVAL);
This means that if second == None we fail, which makes sense, so why store an
Option in Vbios? All methods of Vbios fail if fwsec_image == None.
> + }
> + second
> + };
I think this should be:
let mut second = second_fwsec_image;
if let (Some(second), Some(first), Some(pci_at)) =
(second.as_mut(), first_fwsec_image, pci_at_image)
{
second
.setup_falcon_data(pdev, &pci_at, &first)
.inspect_err(|e| {
dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
})?;
Ok(Vbios(second)
} else {
dev_err!(
pdev.as_ref(),
"Missing required images for falcon data setup, skipping\n"
);
Err(EINVAL)
}
where Vbios can just be
pub(crate) struct Vbios(FwSecBiosImage);
> +
> + Ok(Vbios {
> + fwsec_image: final_fwsec_image,
> + })
> + }
> +
> + pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
> + image.fwsec_header(pdev)
> + }
> +
> + pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
> + image.fwsec_ucode(pdev, image.fwsec_header(pdev)?)
> + }
> +
> + pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
> + image.fwsec_sigs(pdev, image.fwsec_header(pdev)?)
> + }
Those then become infallible, e.g.
pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> &[u8] {
self.0.fwsec_sigs(pdev, self.fwsec_header(pdev))
}
> +}
<snip>
I have to continue with the rest of this patch later on.
- Danilo
Hi Danilo,
On 5/13/2025 1:19 PM, Danilo Krummrich wrote:
> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
>> From: Joel Fernandes <joelagnelf@nvidia.com>
>>
>> Add support for navigating and setting up vBIOS ucode data required for
>> GSP to boot. The main data extracted from the vBIOS is the FWSEC-FRTS
>> firmware which runs on the GSP processor. This firmware runs in high
>> secure mode, and sets up the WPR2 (Write protected region) before the
>> Booter runs on the SEC2 processor.
>>
>> Also add log messages to show the BIOS images.
>>
>> [102141.013287] NovaCore: Found BIOS image at offset 0x0, size: 0xfe00, type: PciAt
>> [102141.080692] NovaCore: Found BIOS image at offset 0xfe00, size: 0x14800, type: Efi
>> [102141.098443] NovaCore: Found BIOS image at offset 0x24600, size: 0x5600, type: FwSec
>> [102141.415095] NovaCore: Found BIOS image at offset 0x29c00, size: 0x60800, type: FwSec
>>
>> Tested on my Ampere GA102 and boot is successful.
>>
>> [applied changes by Alex Courbot for fwsec signatures]
>> [applied feedback from Alex Courbot and Timur Tabi]
>> [applied changes related to code reorg, prints etc from Danilo Krummrich]
>> [acourbot@nvidia.com: fix clippy warnings]
>> [acourbot@nvidia.com: remove now-unneeded Devres acquisition]
>> [acourbot@nvidia.com: fix read_more to read `len` bytes, not u32s]
>>
>> Cc: Alexandre Courbot <acourbot@nvidia.com>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Shirish Baskaran <sbaskaran@nvidia.com>
>> Cc: Alistair Popple <apopple@nvidia.com>
>> Cc: Timur Tabi <ttabi@nvidia.com>
>> Cc: Ben Skeggs <bskeggs@nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> drivers/gpu/nova-core/firmware.rs | 2 -
>> drivers/gpu/nova-core/gpu.rs | 3 +
>> drivers/gpu/nova-core/nova_core.rs | 1 +
>> drivers/gpu/nova-core/vbios.rs | 1147 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 1151 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> index 1eb216307cd01d975b3d5beda1dc516f34b4b3f2..960982174d834c7c66a47ecfb3a15bf47116b2c5 100644
>> --- a/drivers/gpu/nova-core/firmware.rs
>> +++ b/drivers/gpu/nova-core/firmware.rs
>> @@ -80,8 +80,6 @@ pub(crate) struct FalconUCodeDescV3 {
>> _reserved: u16,
>> }
>>
>> -// To be removed once that code is used.
>> -#[expect(dead_code)]
>> impl FalconUCodeDescV3 {
>> pub(crate) fn size(&self) -> usize {
>> ((self.hdr & 0xffff0000) >> 16) as usize
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index ece13594fba687f3f714e255b5436e72d80dece3..4bf7f72247e5320935a517270b5a0e1ec2becfec 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -9,6 +9,7 @@
>> use crate::firmware::Firmware;
>> use crate::regs;
>> use crate::util;
>> +use crate::vbios::Vbios;
>> use core::fmt;
>>
>> macro_rules! define_chipset {
>> @@ -238,6 +239,8 @@ pub(crate) fn new(
>>
>> let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
>>
>> + let _bios = Vbios::new(pdev, bar)?;
>
> Please add a comment why, even though unused, it is important to create this
> instance.
>
> Also, please use `_` if it's not intended to ever be used.
If I add a comment, it will simply be removed by the next patch. I can add that
though so it makes it more clear.
[...]
>> +impl<'a> Iterator for VbiosIterator<'a> {
>> + type Item = Result<BiosImage>;
>> +
>> + /// Iterate over all VBIOS images until the last image is detected or offset
>> + /// exceeds scan limit.
>> + fn next(&mut self) -> Option<Self::Item> {
>> + if self.last_found {
>> + return None;
>> + }
>> +
>> + if self.current_offset > BIOS_MAX_SCAN_LEN {
>> + dev_err!(
>> + self.pdev.as_ref(),
>> + "Error: exceeded BIOS scan limit, stopping scan\n"
>> + );
>> + return None;
>> + }
>> +
>> + // Parse image headers first to get image size
>> + let image_size = match self
>> + .read_bios_image_at_offset(
>> + self.current_offset,
>> + BIOS_READ_AHEAD_SIZE,
>> + "parse initial BIOS image headers",
>> + )
>> + .and_then(|image| image.image_size_bytes())
>> + {
>> + Ok(size) => size,
>> + Err(e) => return Some(Err(e)),
>> + };
>> +
>> + // Now create a new BiosImage with the full image data
>> + let full_image = match self.read_bios_image_at_offset(
>> + self.current_offset,
>> + image_size,
>> + "parse full BIOS image",
>> + ) {
>> + Ok(image) => image,
>> + Err(e) => return Some(Err(e)),
>> + };
>> +
>> + self.last_found = full_image.is_last();
>> +
>> + // Advance to next image (aligned to 512 bytes)
>> + self.current_offset += image_size;
>> + self.current_offset = self.current_offset.align_up(512);
>> +
>> + Some(Ok(full_image))
>> + }
>> +}
>> +
>> +pub(crate) struct Vbios {
>> + pub fwsec_image: Option<FwSecBiosImage>,
>
> Please use pub(crate) instead or provide an accessor.
>
> Also, this shouldn't be an Option, see below comment in Vbios::new().
Ok, I just removed pub altogether, since the users all within this module.
>> +}
>> +
>> +impl Vbios {
>> + /// Probe for VBIOS extraction
>> + /// Once the VBIOS object is built, bar0 is not read for vbios purposes anymore.
>> + pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
>> + // Images to extract from iteration
>> + let mut pci_at_image: Option<PciAtBiosImage> = None;
>> + let mut first_fwsec_image: Option<FwSecBiosImage> = None;
>> + let mut second_fwsec_image: Option<FwSecBiosImage> = None;
>> +
>> + // Parse all VBIOS images in the ROM
>> + for image_result in VbiosIterator::new(pdev, bar0)? {
>> + let full_image = image_result?;
>> +
>> + dev_info!(
>
> Let's use dev_dbg!() instaed.
Done.
>
>> + pdev.as_ref(),
>> + "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
>> + full_image.image_size_bytes()?,
>> + full_image.image_type_str(),
>> + full_image.is_last()
>> + );
>> +
>> + // Get references to images we will need after the loop, in order to
>> + // setup the falcon data offset.
>> + match full_image {
>> + BiosImage::PciAt(image) => {
>> + pci_at_image = Some(image);
>> + }
>> + BiosImage::FwSec(image) => {
>> + if first_fwsec_image.is_none() {
>> + first_fwsec_image = Some(image);
>> + } else {
>> + second_fwsec_image = Some(image);
>> + }
>> + }
>> + // For now we don't need to handle these
>> + BiosImage::Efi(_image) => {}
>> + BiosImage::Nbsi(_image) => {}
>> + }
>> + }
>> +
>> + // Using all the images, setup the falcon data pointer in Fwsec.
>> + // We need mutable access here, so we handle the Option manually.
>> + let final_fwsec_image = {
>> + let mut second = second_fwsec_image; // Take ownership of the option
>> +
>> + if let (Some(second), Some(first), Some(pci_at)) =
>> + (second.as_mut(), first_fwsec_image, pci_at_image)
>> + {
>> + second
>> + .setup_falcon_data(pdev, &pci_at, &first)
>> + .inspect_err(|e| {
>> + dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
>> + })?;
>> + } else {
>> + dev_err!(
>> + pdev.as_ref(),
>> + "Missing required images for falcon data setup, skipping\n"
>> + );
>> + return Err(EINVAL);
>
> This means that if second == None we fail, which makes sense, so why store an
> Option in Vbios? All methods of Vbios fail if fwsec_image == None.
>
Well, if first and pci_at are None, we will fail as well. Not just second. But
we don't know until we finish parsing all the images in the prior loop, if we
found all the images. So we store it as Option during the prior loop, and check
it later. Right?
>> + }
>> + second
>> + };
>
> I think this should be:
>
> let mut second = second_fwsec_image;
>
> if let (Some(second), Some(first), Some(pci_at)) =
> (second.as_mut(), first_fwsec_image, pci_at_image)
> {
> second
> .setup_falcon_data(pdev, &pci_at, &first)
> .inspect_err(|e| {
> dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
> })?;
>
> Ok(Vbios(second)
> } else {
> dev_err!(
> pdev.as_ref(),
> "Missing required images for falcon data setup, skipping\n"
> );
>
> Err(EINVAL)
> }
>
> where Vbios can just be
>
> pub(crate) struct Vbios(FwSecBiosImage);
But your suggestion here still considers second as an Option? That's why you
wrote 'Some(second)' ?
>
>> +
>> + Ok(Vbios {
>> + fwsec_image: final_fwsec_image,
>> + })
>> + }
>> +
>> + pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>> + image.fwsec_header(pdev)
>> + }
>> +
>> + pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>> + image.fwsec_ucode(pdev, image.fwsec_header(pdev)?)
>> + }
>> +
>> + pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>> + image.fwsec_sigs(pdev, image.fwsec_header(pdev)?)
>> + }
>
> Those then become infallible, e.g.
>
> pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> &[u8] {
> self.0.fwsec_sigs(pdev, self.fwsec_header(pdev))
> }
>
Nope, I think you are wrong there. fwsec_sigs() of the underlying .0 returns a
Result.
Also in Vbios::new(), I extract the Option when returning:
Ok(Vbios {
fwsec_image: final_fwsec_image.ok_or(EINVAL)?,
})
But fwsec_header() still has to return a Result:
pub(crate) fn fwsec_header(&self, pdev: &device::Device) ->
Result<FalconUCodeDescV3> {
self.fwsec_image.fwsec_header(pdev)
}
thanks,
- Joel
On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote:
> On 5/13/2025 1:19 PM, Danilo Krummrich wrote:
> > On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
> >> @@ -238,6 +239,8 @@ pub(crate) fn new(
> >>
> >> let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
> >>
> >> + let _bios = Vbios::new(pdev, bar)?;
> >
> > Please add a comment why, even though unused, it is important to create this
> > instance.
> >
> > Also, please use `_` if it's not intended to ever be used.
>
> If I add a comment, it will simply be removed by the next patch. I can add that
> though so it makes it more clear.
I recommend to add such comments, because then reviewers don't stumble over it.
:-)
> >> +impl Vbios {
> >> + /// Probe for VBIOS extraction
> >> + /// Once the VBIOS object is built, bar0 is not read for vbios purposes anymore.
> >> + pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0) -> Result<Vbios> {
> >> + // Images to extract from iteration
> >> + let mut pci_at_image: Option<PciAtBiosImage> = None;
> >> + let mut first_fwsec_image: Option<FwSecBiosImage> = None;
> >> + let mut second_fwsec_image: Option<FwSecBiosImage> = None;
> >> +
> >> + // Parse all VBIOS images in the ROM
> >> + for image_result in VbiosIterator::new(pdev, bar0)? {
> >> + let full_image = image_result?;
> >> +
> >> + dev_info!(
> >
> > Let's use dev_dbg!() instaed.
>
> Done.
>
> >
> >> + pdev.as_ref(),
> >> + "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
> >> + full_image.image_size_bytes()?,
> >> + full_image.image_type_str(),
> >> + full_image.is_last()
> >> + );
> >> +
> >> + // Get references to images we will need after the loop, in order to
> >> + // setup the falcon data offset.
> >> + match full_image {
> >> + BiosImage::PciAt(image) => {
> >> + pci_at_image = Some(image);
> >> + }
> >> + BiosImage::FwSec(image) => {
> >> + if first_fwsec_image.is_none() {
> >> + first_fwsec_image = Some(image);
> >> + } else {
> >> + second_fwsec_image = Some(image);
> >> + }
> >> + }
> >> + // For now we don't need to handle these
> >> + BiosImage::Efi(_image) => {}
> >> + BiosImage::Nbsi(_image) => {}
> >> + }
> >> + }
> >> +
> >> + // Using all the images, setup the falcon data pointer in Fwsec.
> >> + // We need mutable access here, so we handle the Option manually.
> >> + let final_fwsec_image = {
> >> + let mut second = second_fwsec_image; // Take ownership of the option
> >> +
> >> + if let (Some(second), Some(first), Some(pci_at)) =
> >> + (second.as_mut(), first_fwsec_image, pci_at_image)
> >> + {
> >> + second
> >> + .setup_falcon_data(pdev, &pci_at, &first)
> >> + .inspect_err(|e| {
> >> + dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
> >> + })?;
> >> + } else {
> >> + dev_err!(
> >> + pdev.as_ref(),
> >> + "Missing required images for falcon data setup, skipping\n"
> >> + );
> >> + return Err(EINVAL);
> >
> > This means that if second == None we fail, which makes sense, so why store an
> > Option in Vbios? All methods of Vbios fail if fwsec_image == None.
> >
>
> Well, if first and pci_at are None, we will fail as well. Not just second. But
> we don't know until we finish parsing all the images in the prior loop, if we
> found all the images. So we store it as Option during the prior loop, and check
> it later. Right?
My point is not that second is an option within this function -- that's fine. I
don't want the Vbios type to store an Option, because that doesn't make sense.
I.e. it should be
struct Vbios {
fwsec_image: FwSecBiosImage,
}
or just
struct Vbios(FwSecBiosImage);
which is the same, rather than
struct Vbios {
fwsec_image: Option<FwSecBiosImage>,
}
because Vbios::new() fails anyways if any of the images is None, i.e.
vbios.fwsec_image can't ever be None.
The code below does that for you, i.e. it returns an instance of Vbios without
the inner Option.
> >> + }
> >> + second
> >> + };
> >
> > I think this should be:
> >
> > let mut second = second_fwsec_image;
> >
> > if let (Some(second), Some(first), Some(pci_at)) =
> > (second.as_mut(), first_fwsec_image, pci_at_image)
> > {
> > second
> > .setup_falcon_data(pdev, &pci_at, &first)
> > .inspect_err(|e| {
> > dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
> > })?;
> >
> > Ok(Vbios(second)
> > } else {
> > dev_err!(
> > pdev.as_ref(),
> > "Missing required images for falcon data setup, skipping\n"
> > );
> >
> > Err(EINVAL)
> > }
> >
> > where Vbios can just be
> >
> > pub(crate) struct Vbios(FwSecBiosImage);
>
> But your suggestion here still considers second as an Option? That's why you
> wrote 'Some(second)' ?
Yes, that's fine, see above. The difference is that the code returns you an
instance of
struct Vbios(FwSecBiosImage);
rather than
struct Vbios {
fwsec_image: Option<FwSecBiosImage>,
}
which is unnecessary.
>
> >
> >> +
> >> + Ok(Vbios {
> >> + fwsec_image: final_fwsec_image,
> >> + })
> >> + }
> >> +
> >> + pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
> >> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
> >> + image.fwsec_header(pdev)
> >> + }
> >> +
> >> + pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
> >> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
> >> + image.fwsec_ucode(pdev, image.fwsec_header(pdev)?)
> >> + }
> >> +
> >> + pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
> >> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
> >> + image.fwsec_sigs(pdev, image.fwsec_header(pdev)?)
> >> + }
> >
> > Those then become infallible, e.g.
> >
> > pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> &[u8] {
> > self.0.fwsec_sigs(pdev, self.fwsec_header(pdev))
> > }
> >
>
> Nope, I think you are wrong there. fwsec_sigs() of the underlying .0 returns a
> Result.
That's true, I confused self.fwsec_sigs() with self.0.fwsec_sigs(). It seems
that you may want to implement Deref for Vbios.
Also, can you please double check the Options in FwSecBiosImage (in case we
didn't talk about them yet)? They look quite suspicious too.
In general, I feel like a lot of those Option come from a programming pattern
that is very common in C, i.e. allocate a structure (stack or heap) and then
initialize its fields.
In Rust you should aim to initialize all the fields of a structure when you
create the instance. Option as a return type of a function is common, but it's
always a bit suspicious when there is an Option field in a struct.
I understand that there are cases where we can't omit it, and for obvious
reasons the Vbios code is probably a perfect example for that.
However, I recommend looking at this from top to bottom: Do the "final"
structures that we expose to the driver from the Vbios module have fields that
are *really* optional? Or is the Option type just a result from the parsing
process?
If it's the latter, we should get rid of it and work with a different type
during the parsing process and then create the final instance that is exposed to
the driver at the end.
For instance FwSecBiosImage is defined as:
pub(crate) struct FwSecBiosImage {
base: BiosImageBase,
falcon_data_offset: Option<usize>,
pmu_lookup_table: Option<PmuLookupTable>,
falcon_ucode_offset: Option<usize>,
}
Do only *some* FwSecBiosImage instances have a falcon_ucode_offset?
If the answer is 'no' then it shouldn't be an Option. If the answer is 'yes',
then this indicates that FwSecBiosImage is probably too generic and should be
split into more specific types of a FwSecBiosImage which instead share a common
trait in order to treat the different types generically.
> Also in Vbios::new(), I extract the Option when returning:
>
> Ok(Vbios {
> fwsec_image: final_fwsec_image.ok_or(EINVAL)?,
> })
Maybe you do so in your tree? v3 of the patch series has:
pub(crate) struct Vbios {
pub fwsec_image: Option<FwSecBiosImage>,
}
and
Ok(Vbios {
fwsec_image: final_fwsec_image,
})
On 5/20/2025 5:30 AM, Danilo Krummrich wrote:
> On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote:
>> On 5/13/2025 1:19 PM, Danilo Krummrich wrote:
>>> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
>>>> @@ -238,6 +239,8 @@ pub(crate) fn new(
>>>>
>>>> let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
>>>>
>>>> + let _bios = Vbios::new(pdev, bar)?;
>>>
>>> Please add a comment why, even though unused, it is important to create this
>>> instance.
>>>
>>> Also, please use `_` if it's not intended to ever be used.
>>
>> If I add a comment, it will simply be removed by the next patch. I can add that
>> though so it makes it more clear.
>
> I recommend to add such comments, because then reviewers don't stumble over it.
> :-)
Point taken and fixed! ;-)
>>>
>>>> + pdev.as_ref(),
>>>> + "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
>>>> + full_image.image_size_bytes()?,
>>>> + full_image.image_type_str(),
>>>> + full_image.is_last()
>>>> + );
>>>> +
>>>> + // Get references to images we will need after the loop, in order to
>>>> + // setup the falcon data offset.
>>>> + match full_image {
>>>> + BiosImage::PciAt(image) => {
>>>> + pci_at_image = Some(image);
>>>> + }
>>>> + BiosImage::FwSec(image) => {
>>>> + if first_fwsec_image.is_none() {
>>>> + first_fwsec_image = Some(image);
>>>> + } else {
>>>> + second_fwsec_image = Some(image);
>>>> + }
>>>> + }
>>>> + // For now we don't need to handle these
>>>> + BiosImage::Efi(_image) => {}
>>>> + BiosImage::Nbsi(_image) => {}
>>>> + }
>>>> + }
>>>> +
>>>> + // Using all the images, setup the falcon data pointer in Fwsec.
>>>> + // We need mutable access here, so we handle the Option manually.
>>>> + let final_fwsec_image = {
>>>> + let mut second = second_fwsec_image; // Take ownership of the option
>>>> +
>>>> + if let (Some(second), Some(first), Some(pci_at)) =
>>>> + (second.as_mut(), first_fwsec_image, pci_at_image)
>>>> + {
>>>> + second
>>>> + .setup_falcon_data(pdev, &pci_at, &first)
>>>> + .inspect_err(|e| {
>>>> + dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
>>>> + })?;
>>>> + } else {
>>>> + dev_err!(
>>>> + pdev.as_ref(),
>>>> + "Missing required images for falcon data setup, skipping\n"
>>>> + );
>>>> + return Err(EINVAL);
>>>
>>> This means that if second == None we fail, which makes sense, so why store an
>>> Option in Vbios? All methods of Vbios fail if fwsec_image == None.
>>>
>>
>> Well, if first and pci_at are None, we will fail as well. Not just second. But
>> we don't know until we finish parsing all the images in the prior loop, if we
>> found all the images. So we store it as Option during the prior loop, and check
>> it later. Right?
>
> My point is not that second is an option within this function -- that's fine. I
> don't want the Vbios type to store an Option, because that doesn't make sense.
> I.e. it should be
>
> struct Vbios {
> fwsec_image: FwSecBiosImage,
> }
>
> or just
>
> struct Vbios(FwSecBiosImage);
>
> which is the same, rather than
>
> struct Vbios {
> fwsec_image: Option<FwSecBiosImage>,
> }
>
> because Vbios::new() fails anyways if any of the images is None, i.e.
> vbios.fwsec_image can't ever be None.
>
> The code below does that for you, i.e. it returns an instance of Vbios without
> the inner Option.
But your code below does Vbios(second) where Vbios is an option..
>
>>>> + }
>>>> + second
>>>> + };
>>>
>>> I think this should be:
>>>
>>> let mut second = second_fwsec_image;
>>>
>>> if let (Some(second), Some(first), Some(pci_at)) =
>>> (second.as_mut(), first_fwsec_image, pci_at_image)
>>> {
>>> second
>>> .setup_falcon_data(pdev, &pci_at, &first)
>>> .inspect_err(|e| {
>>> dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
>>> })?;
>>>
>>> Ok(Vbios(second)
I can't do that become second is a mutable reference in the above snippet.
But this works:
Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
(This did require changing 'Some(second)' to 'Some(second_ref)', see below.)
>>> } else {
>>> dev_err!(
>>> pdev.as_ref(),
>>> "Missing required images for falcon data setup, skipping\n"
>>> );
>>>
>>> Err(EINVAL)
>>> }
>>>
>>> where Vbios can just be
>>>
>>> pub(crate) struct Vbios(FwSecBiosImage);
>>
>> But your suggestion here still considers second as an Option? That's why you
>> wrote 'Some(second)' ?
>
> Yes, that's fine, see above. The difference is that the code returns you an
> instance of
>
> struct Vbios(FwSecBiosImage);
>
> rather than
>
> struct Vbios {
> fwsec_image: Option<FwSecBiosImage>,
> }
>
> which is unnecessary.
Sure, ok, yeah I made this change in another thread we are discussing so we are
good.
So the code here now looks like the below, definitely better, thanks! :
if let (Some(second_ref), Some(first), Some(pci_at)) =
(second.as_mut(), first_fwsec_image, pci_at_image)
{
second_ref
.setup_falcon_data(pdev, &pci_at, &first)
.inspect_err(|e| {
dev_err!(..)
})?;
Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
} else {
dev_err!(
pdev.as_ref(),
"Missing required images for falcon data setup, skipping\n"
);
Err(EINVAL)
}
>>>> +
>>>> + Ok(Vbios {
>>>> + fwsec_image: final_fwsec_image,
>>>> + })
>>>> + }
>>>> +
>>>> + pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
>>>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>>>> + image.fwsec_header(pdev)
>>>> + }
>>>> +
>>>> + pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
>>>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>>>> + image.fwsec_ucode(pdev, image.fwsec_header(pdev)?)
>>>> + }
>>>> +
>>>> + pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
>>>> + let image = self.fwsec_image.as_ref().ok_or(EINVAL)?;
>>>> + image.fwsec_sigs(pdev, image.fwsec_header(pdev)?)
>>>> + }
>>>
>>> Those then become infallible, e.g.
>>>
>>> pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> &[u8] {
>>> self.0.fwsec_sigs(pdev, self.fwsec_header(pdev))
>>> }
>>>
>>
>> Nope, I think you are wrong there. fwsec_sigs() of the underlying .0 returns a
>> Result.
>
> That's true, I confused self.fwsec_sigs() with self.0.fwsec_sigs(). It seems
> that you may want to implement Deref for Vbios.
>
> Also, can you please double check the Options in FwSecBiosImage (in case we
> didn't talk about them yet)? They look quite suspicious too.
> In general, I feel like a lot of those Option come from a programming pattern
> that is very common in C, i.e. allocate a structure (stack or heap) and then
> initialize its fields.
>
> In Rust you should aim to initialize all the fields of a structure when you
> create the instance. Option as a return type of a function is common, but it's
> always a bit suspicious when there is an Option field in a struct.
I looked into it, I could not git rid of those ones because we need to
initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"
0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
base,
falcon_data_offset: None,
pmu_lookup_table: None,
falcon_ucode_offset: None,
})),
And these fields will not be determined until much later, because as is the case
with the earlier example, these fields cannot be determined until all the images
are parsed.
> I understand that there are cases where we can't omit it, and for obvious
> reasons the Vbios code is probably a perfect example for that.
>
> However, I recommend looking at this from top to bottom: Do the "final"
> structures that we expose to the driver from the Vbios module have fields that
> are *really* optional? Or is the Option type just a result from the parsing
> process?
>
> If it's the latter, we should get rid of it and work with a different type
> during the parsing process and then create the final instance that is exposed to
> the driver at the end.
>
> For instance FwSecBiosImage is defined as:
>
> pub(crate) struct FwSecBiosImage {
> base: BiosImageBase,
> falcon_data_offset: Option<usize>,
> pmu_lookup_table: Option<PmuLookupTable>,
> falcon_ucode_offset: Option<usize>,
> }
>
> Do only *some* FwSecBiosImage instances have a falcon_ucode_offset?
>
> If the answer is 'no' then it shouldn't be an Option. If the answer is 'yes',
> then this indicates that FwSecBiosImage is probably too generic and should be
> split into more specific types of a FwSecBiosImage which instead share a common
> trait in order to treat the different types generically.
Understood, thanks.
>> Also in Vbios::new(), I extract the Option when returning:
>>
>> Ok(Vbios {
>> fwsec_image: final_fwsec_image.ok_or(EINVAL)?,
>> })
>
> Maybe you do so in your tree? v3 of the patch series has:
>
> pub(crate) struct Vbios {
> pub fwsec_image: Option<FwSecBiosImage>,
> }
>
> and
>
> Ok(Vbios {
> fwsec_image: final_fwsec_image,
> })
Yes, I made the change during our review on the other thread and will be posted
in the next posting. Sorry for any confusion.
thanks,
- Joel
On Tue, May 20, 2025 at 09:43:42AM -0400, Joel Fernandes wrote:
> On 5/20/2025 5:30 AM, Danilo Krummrich wrote:
> > On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote:
> >> On 5/13/2025 1:19 PM, Danilo Krummrich wrote:
> >>> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
>
> So the code here now looks like the below, definitely better, thanks! :
>
> if let (Some(second_ref), Some(first), Some(pci_at)) =
> (second.as_mut(), first_fwsec_image, pci_at_image)
> {
> second_ref
> .setup_falcon_data(pdev, &pci_at, &first)
> .inspect_err(|e| {
> dev_err!(..)
> })?;
> Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
> } else {
> dev_err!(
> pdev.as_ref(),
> "Missing required images for falcon data setup, skipping\n"
> );
> Err(EINVAL)
> }
Sorry, my code-snipped was incorrect indeed. Let me paste what I actually
intended (and this time properly compile checked) and should be even better:
if let (Some(mut second), Some(first), Some(pci_at)) =
(second_fwsec_image, first_fwsec_image, pci_at_image)
{
second
.setup_falcon_data(pdev, &pci_at, &first)
.inspect_err(|e| {
dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
})?;
Ok(Vbios(second))
} else {
dev_err!(
pdev.as_ref(),
"Missing required images for falcon data setup, skipping\n"
);
Err(EINVAL)
}
So, with this second is the actual value and not just a reference. :)
And the methods can become:
pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
self.0.fwsec_header(pdev)
}
pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
self.0.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
}
pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
self.0.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
}
However, I don't understand why they're not just implemented for FwSecBiosImage
itself this way. You can just implement Deref for Vbios then.
> > In general, I feel like a lot of those Option come from a programming pattern
> > that is very common in C, i.e. allocate a structure (stack or heap) and then
> > initialize its fields.
> >
> > In Rust you should aim to initialize all the fields of a structure when you
> > create the instance. Option as a return type of a function is common, but it's
> > always a bit suspicious when there is an Option field in a struct.
>
> I looked into it, I could not git rid of those ones because we need to
> initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"
>
> 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
> base,
> falcon_data_offset: None,
> pmu_lookup_table: None,
> falcon_ucode_offset: None,
> })),
>
> And these fields will not be determined until much later, because as is the case
> with the earlier example, these fields cannot be determined until all the images
> are parsed.
You should not use TryFrom, but instead use a normal constructor, such as
BiosImage::new(base_bios_image)
and do the parsing within this constructor.
If you want a helper type with Options while parsing that's totally fine, but
the final result can clearly be without Options. For instance:
struct Data {
image: KVec<u8>,
}
impl Data {
fn new() -> Result<Self> {
let parser = DataParser::new();
Self { image: parser.parse()? }
}
fn load_image(&self) {
...
}
}
struct DataParser {
// Only some images have a checksum.
checksum: Option<u64>,
// Some images have an extra offset.
offset: Option<u64>,
// Some images need to be patched.
patch: Option<KVec<u8>>,
image: KVec<u8>,
}
impl DataParser {
fn new() -> Self {
Self {
checksum: None,
offset: None,
patch: None,
bytes: KVec::new(),
}
}
fn parse(self) -> Result<KVec<u8>> {
// Fetch all the required data.
self.fetch_checksum()?;
self.fetch_offset()?;
self.fetch_patch()?;
self.fetch_byes()?;
// Doesn't do anything if `checksum == None`.
self.validate_checksum()?;
// Doesn't do anything if `offset == None`.
self.apply_offset()?;
// Doesn't do anything if `patch == None`.
self.apply_patch()?;
// Return the final image.
self.image
}
}
I think the pattern here is the same, but in this example you keep working with
the DataParser, instead of a new instance of Data.
> > I understand that there are cases where we can't omit it, and for obvious
> > reasons the Vbios code is probably a perfect example for that.
> >
> > However, I recommend looking at this from top to bottom: Do the "final"
> > structures that we expose to the driver from the Vbios module have fields that
> > are *really* optional? Or is the Option type just a result from the parsing
> > process?
> >
> > If it's the latter, we should get rid of it and work with a different type
> > during the parsing process and then create the final instance that is exposed to
> > the driver at the end.
> >
> > For instance FwSecBiosImage is defined as:
> >
> > pub(crate) struct FwSecBiosImage {
> > base: BiosImageBase,
> > falcon_data_offset: Option<usize>,
> > pmu_lookup_table: Option<PmuLookupTable>,
> > falcon_ucode_offset: Option<usize>,
> > }
> >
> > Do only *some* FwSecBiosImage instances have a falcon_ucode_offset?
> >
> > If the answer is 'no' then it shouldn't be an Option. If the answer is 'yes',
> > then this indicates that FwSecBiosImage is probably too generic and should be
> > split into more specific types of a FwSecBiosImage which instead share a common
> > trait in order to treat the different types generically.
>
> Understood, thanks.
On 5/20/2025 11:01 AM, Danilo Krummrich wrote:
> On Tue, May 20, 2025 at 09:43:42AM -0400, Joel Fernandes wrote:
>> On 5/20/2025 5:30 AM, Danilo Krummrich wrote:
>>> On Tue, May 20, 2025 at 03:55:06AM -0400, Joel Fernandes wrote:
>>>> On 5/13/2025 1:19 PM, Danilo Krummrich wrote:
>>>>> On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot wrote:
>>
>> So the code here now looks like the below, definitely better, thanks! :
>>
>> if let (Some(second_ref), Some(first), Some(pci_at)) =
>> (second.as_mut(), first_fwsec_image, pci_at_image)
>> {
>> second_ref
>> .setup_falcon_data(pdev, &pci_at, &first)
>> .inspect_err(|e| {
>> dev_err!(..)
>> })?;
>> Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
>> } else {
>> dev_err!(
>> pdev.as_ref(),
>> "Missing required images for falcon data setup, skipping\n"
>> );
>> Err(EINVAL)
>> }
>
> Sorry, my code-snipped was incorrect indeed. Let me paste what I actually
> intended (and this time properly compile checked) and should be even better:
>
> if let (Some(mut second), Some(first), Some(pci_at)) =
> (second_fwsec_image, first_fwsec_image, pci_at_image)
> {
> second
> .setup_falcon_data(pdev, &pci_at, &first)
> .inspect_err(|e| {
> dev_err!(pdev.as_ref(), "Falcon data setup failed: {:?}\n", e)
> })?;
>
> Ok(Vbios(second))
> } else {
> dev_err!(
> pdev.as_ref(),
> "Missing required images for falcon data setup, skipping\n"
> );
>
> Err(EINVAL)
> }
>
> So, with this second is the actual value and not just a reference. :)
>
> And the methods can become:
>
> pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
> self.0.fwsec_header(pdev)
> }
>
> pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
> self.0.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
> }
>
> pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
> self.0.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
> }
I made this change and it LGTM. Thanks! I did not do the '.0' though since I
want to keep the readability, lets see in the next revision if that looks good.
>>> In general, I feel like a lot of those Option come from a programming pattern
>>> that is very common in C, i.e. allocate a structure (stack or heap) and then
>>> initialize its fields.
>>>
>>> In Rust you should aim to initialize all the fields of a structure when you
>>> create the instance. Option as a return type of a function is common, but it's
>>> always a bit suspicious when there is an Option field in a struct.
>>
>> I looked into it, I could not git rid of those ones because we need to
>> initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"
>>
>> 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
>> base,
>> falcon_data_offset: None,
>> pmu_lookup_table: None,
>> falcon_ucode_offset: None,
>> })),
>>
>> And these fields will not be determined until much later, because as is the case
>> with the earlier example, these fields cannot be determined until all the images
>> are parsed.
>
> You should not use TryFrom, but instead use a normal constructor, such as
>
> BiosImage::new(base_bios_image)
>
> and do the parsing within this constructor.
>
> If you want a helper type with Options while parsing that's totally fine, but
> the final result can clearly be without Options. For instance:
>
> struct Data {
> image: KVec<u8>,
> }
>
> impl Data {
> fn new() -> Result<Self> {
> let parser = DataParser::new();
>
> Self { image: parser.parse()? }
> }
>
> fn load_image(&self) {
> ...
> }
> }
>
> struct DataParser {
> // Only some images have a checksum.
> checksum: Option<u64>,
> // Some images have an extra offset.
> offset: Option<u64>,
> // Some images need to be patched.
> patch: Option<KVec<u8>>,
> image: KVec<u8>,
> }
>
> impl DataParser {
> fn new() -> Self {
> Self {
> checksum: None,
> offset: None,
> patch: None,
> bytes: KVec::new(),
> }
> }
>
> fn parse(self) -> Result<KVec<u8>> {
> // Fetch all the required data.
> self.fetch_checksum()?;
> self.fetch_offset()?;
> self.fetch_patch()?;
> self.fetch_byes()?;
>
> // Doesn't do anything if `checksum == None`.
> self.validate_checksum()?;
>
> // Doesn't do anything if `offset == None`.
> self.apply_offset()?;
>
> // Doesn't do anything if `patch == None`.
> self.apply_patch()?;
>
> // Return the final image.
> self.image
> }
> }
>
> I think the pattern here is the same, but in this example you keep working with
> the DataParser, instead of a new instance of Data.
I think this would be a fundamental rewrite of the patch. I am Ok with looking
into it as a future item, but right now I am not sure if it justifies not using
Option for these few. There's a lot of immediate work we have to do for boot,
lets please not block the patch on just this if that's Ok with you. If you want,
I could add a TODO here.
thanks,
- Joel
On Tue, May 20, 2025 at 11:11:12AM -0400, Joel Fernandes wrote:
> On 5/20/2025 11:01 AM, Danilo Krummrich wrote:
>
> I made this change and it LGTM. Thanks! I did not do the '.0' though since I
> want to keep the readability, lets see in the next revision if that looks good.
I think readability, is just as good with `.0`, but I'm fine with either.
> >>> In general, I feel like a lot of those Option come from a programming pattern
> >>> that is very common in C, i.e. allocate a structure (stack or heap) and then
> >>> initialize its fields.
> >>>
> >>> In Rust you should aim to initialize all the fields of a structure when you
> >>> create the instance. Option as a return type of a function is common, but it's
> >>> always a bit suspicious when there is an Option field in a struct.
> >>
> >> I looked into it, I could not git rid of those ones because we need to
> >> initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"
> >>
> >> 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
> >> base,
> >> falcon_data_offset: None,
> >> pmu_lookup_table: None,
> >> falcon_ucode_offset: None,
> >> })),
> >>
> >> And these fields will not be determined until much later, because as is the case
> >> with the earlier example, these fields cannot be determined until all the images
> >> are parsed.
> >
> > You should not use TryFrom, but instead use a normal constructor, such as
> >
> > BiosImage::new(base_bios_image)
> >
> > and do the parsing within this constructor.
> >
> > If you want a helper type with Options while parsing that's totally fine, but
> > the final result can clearly be without Options. For instance:
> >
> > struct Data {
> > image: KVec<u8>,
> > }
> >
> > impl Data {
> > fn new() -> Result<Self> {
> > let parser = DataParser::new();
> >
> > Self { image: parser.parse()? }
> > }
> >
> > fn load_image(&self) {
> > ...
> > }
> > }
> >
> > struct DataParser {
> > // Only some images have a checksum.
> > checksum: Option<u64>,
> > // Some images have an extra offset.
> > offset: Option<u64>,
> > // Some images need to be patched.
> > patch: Option<KVec<u8>>,
> > image: KVec<u8>,
> > }
> >
> > impl DataParser {
> > fn new() -> Self {
> > Self {
> > checksum: None,
> > offset: None,
> > patch: None,
> > bytes: KVec::new(),
> > }
> > }
> >
> > fn parse(self) -> Result<KVec<u8>> {
> > // Fetch all the required data.
> > self.fetch_checksum()?;
> > self.fetch_offset()?;
> > self.fetch_patch()?;
> > self.fetch_byes()?;
> >
> > // Doesn't do anything if `checksum == None`.
> > self.validate_checksum()?;
> >
> > // Doesn't do anything if `offset == None`.
> > self.apply_offset()?;
> >
> > // Doesn't do anything if `patch == None`.
> > self.apply_patch()?;
> >
> > // Return the final image.
> > self.image
> > }
> > }
> >
> > I think the pattern here is the same, but in this example you keep working with
> > the DataParser, instead of a new instance of Data.
>
> I think this would be a fundamental rewrite of the patch. I am Ok with looking
> into it as a future item, but right now I am not sure if it justifies not using
> Option for these few. There's a lot of immediate work we have to do for boot,
> lets please not block the patch on just this if that's Ok with you. If you want,
> I could add a TODO here.
Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
juggling fields and moving code around. The actual code should not change much.
Having Option<T> where the corresponding value T isn't actually optional is
extremely confusing and makes it hard for everyone, but especially new
contributors, to understand the code and can easily trick people into taking
wrong assumptions.
Making the code reasonably accessible for (new) contributors is one of the
objectives of nova and one of the learnings from nouveau.
Hence, let's get this right from the get-go please.
On 5/20/2025 11:36 AM, Danilo Krummrich wrote:
>>> If you want a helper type with Options while parsing that's totally fine, but
>>> the final result can clearly be without Options. For instance:
>>>
>>> struct Data {
>>> image: KVec<u8>,
>>> }
>>>
>>> impl Data {
>>> fn new() -> Result<Self> {
>>> let parser = DataParser::new();
>>>
>>> Self { image: parser.parse()? }
>>> }
>>>
>>> fn load_image(&self) {
>>> ...
>>> }
>>> }
>>>
>>> struct DataParser {
>>> // Only some images have a checksum.
>>> checksum: Option<u64>,
>>> // Some images have an extra offset.
>>> offset: Option<u64>,
>>> // Some images need to be patched.
>>> patch: Option<KVec<u8>>,
>>> image: KVec<u8>,
>>> }
>>>
>>> impl DataParser {
>>> fn new() -> Self {
>>> Self {
>>> checksum: None,
>>> offset: None,
>>> patch: None,
>>> bytes: KVec::new(),
>>> }
>>> }
>>>
>>> fn parse(self) -> Result<KVec<u8>> {
>>> // Fetch all the required data.
>>> self.fetch_checksum()?;
>>> self.fetch_offset()?;
>>> self.fetch_patch()?;
>>> self.fetch_byes()?;
>>>
>>> // Doesn't do anything if `checksum == None`.
>>> self.validate_checksum()?;
>>>
>>> // Doesn't do anything if `offset == None`.
>>> self.apply_offset()?;
>>>
>>> // Doesn't do anything if `patch == None`.
>>> self.apply_patch()?;
>>>
>>> // Return the final image.
>>> self.image
>>> }
>>> }
>>>
>>> I think the pattern here is the same, but in this example you keep working with
>>> the DataParser, instead of a new instance of Data.
>> I think this would be a fundamental rewrite of the patch. I am Ok with looking
>> into it as a future item, but right now I am not sure if it justifies not using
>> Option for these few. There's a lot of immediate work we have to do for boot,
>> lets please not block the patch on just this if that's Ok with you. If you want,
>> I could add a TODO here.
>
> Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
> juggling fields and moving code around. The actual code should not change much.
>
> Having Option<T> where the corresponding value T isn't actually optional is
> extremely confusing and makes it hard for everyone, but especially new
> contributors, to understand the code and can easily trick people into taking
> wrong assumptions.
>
> Making the code reasonably accessible for (new) contributors is one of the
> objectives of nova and one of the learnings from nouveau.
I implemented the Data parsing pattern like the following, the final
FwSecBiosImage will not have optional fields as you suggested. It does get rid
of 2 additional fields as well which are not needed after vbios parsing completes.
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=nova/vbios&id=8cc852fe5573890596a91a2a935b3af24dcb9f04
Hope that looks Ok now! I am open to naming FwSecBiosPartial as FwSecBiosData if
that's better.
The full file after the change:
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/tree/drivers/gpu/nova-core/vbios.rs?h=nova/vbios&id=8cc852fe5573890596a91a2a935b3af24dcb9f04
thanks,
- Joel
On Wed, 21 May 2025 at 04:13, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
>
>
> On 5/20/2025 11:36 AM, Danilo Krummrich wrote:
> >>> If you want a helper type with Options while parsing that's totally fine, but
> >>> the final result can clearly be without Options. For instance:
> >>>
> >>> struct Data {
> >>> image: KVec<u8>,
> >>> }
> >>>
> >>> impl Data {
> >>> fn new() -> Result<Self> {
> >>> let parser = DataParser::new();
> >>>
> >>> Self { image: parser.parse()? }
> >>> }
> >>>
> >>> fn load_image(&self) {
> >>> ...
> >>> }
> >>> }
> >>>
> >>> struct DataParser {
> >>> // Only some images have a checksum.
> >>> checksum: Option<u64>,
> >>> // Some images have an extra offset.
> >>> offset: Option<u64>,
> >>> // Some images need to be patched.
> >>> patch: Option<KVec<u8>>,
> >>> image: KVec<u8>,
> >>> }
> >>>
> >>> impl DataParser {
> >>> fn new() -> Self {
> >>> Self {
> >>> checksum: None,
> >>> offset: None,
> >>> patch: None,
> >>> bytes: KVec::new(),
> >>> }
> >>> }
> >>>
> >>> fn parse(self) -> Result<KVec<u8>> {
> >>> // Fetch all the required data.
> >>> self.fetch_checksum()?;
> >>> self.fetch_offset()?;
> >>> self.fetch_patch()?;
> >>> self.fetch_byes()?;
> >>>
> >>> // Doesn't do anything if `checksum == None`.
> >>> self.validate_checksum()?;
> >>>
> >>> // Doesn't do anything if `offset == None`.
> >>> self.apply_offset()?;
> >>>
> >>> // Doesn't do anything if `patch == None`.
> >>> self.apply_patch()?;
> >>>
> >>> // Return the final image.
> >>> self.image
> >>> }
> >>> }
> >>>
> >>> I think the pattern here is the same, but in this example you keep working with
> >>> the DataParser, instead of a new instance of Data.
> >> I think this would be a fundamental rewrite of the patch. I am Ok with looking
> >> into it as a future item, but right now I am not sure if it justifies not using
> >> Option for these few. There's a lot of immediate work we have to do for boot,
> >> lets please not block the patch on just this if that's Ok with you. If you want,
> >> I could add a TODO here.
> >
> > Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
> > juggling fields and moving code around. The actual code should not change much.
> >
> > Having Option<T> where the corresponding value T isn't actually optional is
> > extremely confusing and makes it hard for everyone, but especially new
> > contributors, to understand the code and can easily trick people into taking
> > wrong assumptions.
> >
> > Making the code reasonably accessible for (new) contributors is one of the
> > objectives of nova and one of the learnings from nouveau.
I just want to back Danilo up on this concept as well.
When I did the experiments code, I faced the not fully constructed
object problem a lot, and I tried to resist the C pattern of Option<>
all the things, it's a very C based thing where we create an object
then initialise it as we go, and it's not a great pattern to have for
rust code.
I'm not a huge fan of constructor/builder objects either if they can
be avoided, please do, and I tried to also avoid proliferating them,
but I think for most things we can build the pieces and then the final
object as we go, it just requires doing so from the start, and not
giving into the Option<> pattern.
Dave.
On 5/20/2025 5:32 PM, Dave Airlie wrote:
> On Wed, 21 May 2025 at 04:13, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>>
>>
>> On 5/20/2025 11:36 AM, Danilo Krummrich wrote:
>>>>> If you want a helper type with Options while parsing that's totally fine, but
>>>>> the final result can clearly be without Options. For instance:
>>>>>
>>>>> struct Data {
>>>>> image: KVec<u8>,
>>>>> }
>>>>>
>>>>> impl Data {
>>>>> fn new() -> Result<Self> {
>>>>> let parser = DataParser::new();
>>>>>
>>>>> Self { image: parser.parse()? }
>>>>> }
>>>>>
>>>>> fn load_image(&self) {
>>>>> ...
>>>>> }
>>>>> }
>>>>>
>>>>> struct DataParser {
>>>>> // Only some images have a checksum.
>>>>> checksum: Option<u64>,
>>>>> // Some images have an extra offset.
>>>>> offset: Option<u64>,
>>>>> // Some images need to be patched.
>>>>> patch: Option<KVec<u8>>,
>>>>> image: KVec<u8>,
>>>>> }
>>>>>
>>>>> impl DataParser {
>>>>> fn new() -> Self {
>>>>> Self {
>>>>> checksum: None,
>>>>> offset: None,
>>>>> patch: None,
>>>>> bytes: KVec::new(),
>>>>> }
>>>>> }
>>>>>
>>>>> fn parse(self) -> Result<KVec<u8>> {
>>>>> // Fetch all the required data.
>>>>> self.fetch_checksum()?;
>>>>> self.fetch_offset()?;
>>>>> self.fetch_patch()?;
>>>>> self.fetch_byes()?;
>>>>>
>>>>> // Doesn't do anything if `checksum == None`.
>>>>> self.validate_checksum()?;
>>>>>
>>>>> // Doesn't do anything if `offset == None`.
>>>>> self.apply_offset()?;
>>>>>
>>>>> // Doesn't do anything if `patch == None`.
>>>>> self.apply_patch()?;
>>>>>
>>>>> // Return the final image.
>>>>> self.image
>>>>> }
>>>>> }
>>>>>
>>>>> I think the pattern here is the same, but in this example you keep working with
>>>>> the DataParser, instead of a new instance of Data.
>>>> I think this would be a fundamental rewrite of the patch. I am Ok with looking
>>>> into it as a future item, but right now I am not sure if it justifies not using
>>>> Option for these few. There's a lot of immediate work we have to do for boot,
>>>> lets please not block the patch on just this if that's Ok with you. If you want,
>>>> I could add a TODO here.
>>>
>>> Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
>>> juggling fields and moving code around. The actual code should not change much.
>>>
>>> Having Option<T> where the corresponding value T isn't actually optional is
>>> extremely confusing and makes it hard for everyone, but especially new
>>> contributors, to understand the code and can easily trick people into taking
>>> wrong assumptions.
>>>
>>> Making the code reasonably accessible for (new) contributors is one of the
>>> objectives of nova and one of the learnings from nouveau.
>
> I just want to back Danilo up on this concept as well.
>
> When I did the experiments code, I faced the not fully constructed
> object problem a lot, and I tried to resist the C pattern of Option<>
> all the things, it's a very C based thing where we create an object
> then initialise it as we go, and it's not a great pattern to have for
> rust code.
>
> I'm not a huge fan of constructor/builder objects either if they can
> be avoided, please do, and I tried to also avoid proliferating them,
> but I think for most things we can build the pieces and then the final
> object as we go, it just requires doing so from the start, and not
> giving into the Option<> pattern.
Sure, I am on the same page there as well. For next revision of this patch,
struct Vbios will contain a struct FwsecBiosImage without any Option in either
struct Vbios or struct FwsecBiosImage. This is only logical, because there is
nothing optional about it (in what Vbios::new() returns).
thanks,
- Joel
> On May 20, 2025, at 11:37 AM, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, May 20, 2025 at 11:11:12AM -0400, Joel Fernandes wrote:
>> On 5/20/2025 11:01 AM, Danilo Krummrich wrote:
>>
>> I made this change and it LGTM. Thanks! I did not do the '.0' though since I
>> want to keep the readability, lets see in the next revision if that looks good.
>
> I think readability, is just as good with `.0`, but I'm fine with either.
Cool.
>
>>>>> In general, I feel like a lot of those Option come from a programming pattern
>>>>> that is very common in C, i.e. allocate a structure (stack or heap) and then
>>>>> initialize its fields.
>>>>>
>>>>> In Rust you should aim to initialize all the fields of a structure when you
>>>>> create the instance. Option as a return type of a function is common, but it's
>>>>> always a bit suspicious when there is an Option field in a struct.
>>>>
>>>> I looked into it, I could not git rid of those ones because we need to
>>>> initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"
>>>>
>>>> 0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
>>>> base,
>>>> falcon_data_offset: None,
>>>> pmu_lookup_table: None,
>>>> falcon_ucode_offset: None,
>>>> })),
>>>>
>>>> And these fields will not be determined until much later, because as is the case
>>>> with the earlier example, these fields cannot be determined until all the images
>>>> are parsed.
>>>
>>> You should not use TryFrom, but instead use a normal constructor, such as
>>>
>>> BiosImage::new(base_bios_image)
>>>
>>> and do the parsing within this constructor.
>>>
>>> If you want a helper type with Options while parsing that's totally fine, but
>>> the final result can clearly be without Options. For instance:
>>>
>>> struct Data {
>>> image: KVec<u8>,
>>> }
>>>
>>> impl Data {
>>> fn new() -> Result<Self> {
>>> let parser = DataParser::new();
>>>
>>> Self { image: parser.parse()? }
>>> }
>>>
>>> fn load_image(&self) {
>>> ...
>>> }
>>> }
>>>
>>> struct DataParser {
>>> // Only some images have a checksum.
>>> checksum: Option<u64>,
>>> // Some images have an extra offset.
>>> offset: Option<u64>,
>>> // Some images need to be patched.
>>> patch: Option<KVec<u8>>,
>>> image: KVec<u8>,
>>> }
>>>
>>> impl DataParser {
>>> fn new() -> Self {
>>> Self {
>>> checksum: None,
>>> offset: None,
>>> patch: None,
>>> bytes: KVec::new(),
>>> }
>>> }
>>>
>>> fn parse(self) -> Result<KVec<u8>> {
>>> // Fetch all the required data.
>>> self.fetch_checksum()?;
>>> self.fetch_offset()?;
>>> self.fetch_patch()?;
>>> self.fetch_byes()?;
>>>
>>> // Doesn't do anything if `checksum == None`.
>>> self.validate_checksum()?;
>>>
>>> // Doesn't do anything if `offset == None`.
>>> self.apply_offset()?;
>>>
>>> // Doesn't do anything if `patch == None`.
>>> self.apply_patch()?;
>>>
>>> // Return the final image.
>>> self.image
>>> }
>>> }
>>>
>>> I think the pattern here is the same, but in this example you keep working with
>>> the DataParser, instead of a new instance of Data.
>>
>> I think this would be a fundamental rewrite of the patch. I am Ok with looking
>> into it as a future item, but right now I am not sure if it justifies not using
>> Option for these few. There's a lot of immediate work we have to do for boot,
>> lets please not block the patch on just this if that's Ok with you. If you want,
>> I could add a TODO here.
>
> Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
> juggling fields and moving code around. The actual code should not change much.
>
> Having Option<T> where the corresponding value T isn't actually optional is
> extremely confusing and makes it hard for everyone, but especially new
> contributors, to understand the code and can easily trick people into taking
> wrong assumptions.
>
> Making the code reasonably accessible for (new) contributors is one of the
> objectives of nova and one of the learnings from nouveau.
>
> Hence, let's get this right from the get-go please.
Ok, I will look into making this change. :-)
thanks,
- Joel
© 2016 - 2025 Red Hat, Inc.