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
