Re: [PATCH v4 20/20] gpu: nova-core: vbios: move constants and functions to be associated

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> Move constants and functions to be inside the impls of the types they
> are related to. This makes it more obvious what each type and value is
> for.
> 
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  Documentation/gpu/nova/core/vbios.rst |   2 +-
>  drivers/gpu/nova-core/vbios.rs        | 185 +++++++++++++++++-----------------
>  2 files changed, 96 insertions(+), 91 deletions(-)

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

thanks,
-- 
John Hubbard


> 
> diff --git a/Documentation/gpu/nova/core/vbios.rst b/Documentation/gpu/nova/core/vbios.rst
> index a4fe63422ede..9d3379ccfb30 100644
> --- a/Documentation/gpu/nova/core/vbios.rst
> +++ b/Documentation/gpu/nova/core/vbios.rst
> @@ -232,7 +232,7 @@ Falcon data in the VBIOS which contains the PMU lookup table. This lookup table
>  used to find the required Falcon ucode based on an application ID.
>  
>  The location of the PMU lookup table is found by scanning the BIT (`BIOS Information Table`_)
> -tokens for a token with the id `BIT_TOKEN_ID_FALCON_DATA` (0x70) which indicates the
> +tokens for a token with the Falcon data token id (0x70) which indicates the
>  offset of the same from the start of the VBIOS image. Unfortunately, the offset
>  does not account for the EFI image located between the PciAt and FwSec images.
>  The `vbios.rs` code compensates for this with appropriate arithmetic.
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 9cc2f008bbfb..07b5235faff8 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -27,16 +27,6 @@
>      num::FromSafeCast,
>  };
>  
> -/// 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;
> -
>  /// BIOS Image Type from PCI Data Structure code_type field.
>  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>  #[repr(u8)]
> @@ -65,14 +55,6 @@ fn try_from(code: u8) -> Result<Self> {
>      }
>  }
>  
> -// 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> {
>      dev: &'a device::Device,
> @@ -110,73 +92,79 @@ struct VbiosIterator<'a> {
>      }
>  }
>  
> -/// Return the byte offset where the PCI Expansion ROM images begin in the GPU's ROM.
> -///
> -/// The GPU's ROM may begin with an Init-from-ROM (IFR) header that precedes
> -/// the PCI Expansion ROM images (VBIOS).  When present, the PROM shadow
> -/// method must parse this header to determine the offset where the PCI ROM
> -/// images actually begin, and adjust all subsequent reads accordingly.
> -///
> -/// On most GPUs this is not needed because the IFR microcode has already
> -/// applied the ROM offset so that PROM reads transparently skip the header.
> -/// On GA100, for some reason, the IFR offset is not applied to PROM
> -/// reads.  Therefore, the search for the PCI expansion must skip the IFR
> -/// header, if found.
> -fn vbios_rom_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {
> -    /// IFR signature.
> -    const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");
> -    /// ROM directory signature.
> -    const NV_ROM_DIRECTORY_IDENTIFIER: u32 = u32::from_le_bytes(*b"RFRD");
> -    /// Offset of the NV_PMGR_ROM_ADDR_OFFSET register in IFR Extended section.
> -    const IFR_SW_EXT_ROM_ADDR_OFFSET: usize = 4;
> -    /// Size of Redundant Firmware Flash Status section.
> -    const RFW_FLASH_STATUS_SIZE: usize = SZ_4K;
> -    /// Offset in the ROM Directory of the PCI Option ROM offset
> -    const PCI_OPTION_ROM_OFFSET: usize = 8;
> -
> -    let signature = bar0.read(NV_PBUS_IFR_FMT_FIXED0).signature();
> -
> -    if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
> -        let fixed1 = bar0.read(NV_PBUS_IFR_FMT_FIXED1);
> -
> -        match fixed1.version() {
> -            1 | 2 => {
> -                let fixed_data_size = usize::from(fixed1.fixed_data_size());
> -                let pmgr_rom_addr_offset = fixed_data_size + IFR_SW_EXT_ROM_ADDR_OFFSET;
> -                bar0.try_read32(ROM_OFFSET + pmgr_rom_addr_offset)
> -                    .map(usize::from_safe_cast)
> -            }
> -            3 => {
> -                let fixed2 = bar0.read(NV_PBUS_IFR_FMT_FIXED2);
> -                let total_data_size = usize::from(fixed2.total_data_size());
> -                let flash_status_offset =
> -                    usize::from_safe_cast(bar0.try_read32(ROM_OFFSET + total_data_size)?);
> -                let dir_offset = flash_status_offset + RFW_FLASH_STATUS_SIZE;
> -                let dir_sig = bar0.try_read32(ROM_OFFSET + dir_offset)?;
> -                if dir_sig != NV_ROM_DIRECTORY_IDENTIFIER {
> -                    dev_err!(dev, "could not find IFR ROM directory\n");
> -                    return Err(EINVAL);
> -                }
> -                bar0.try_read32(ROM_OFFSET + dir_offset + PCI_OPTION_ROM_OFFSET)
> -                    .map(usize::from_safe_cast)
> -            }
> -            _ => {
> -                dev_err!(dev, "unsupported IFR header version {}\n", fixed1.version());
> -                Err(EINVAL)
> -            }
> -        }
> -    } else {
> -        Ok(0)
> -    }
> -}
> -
>  impl<'a> VbiosIterator<'a> {
> +    /// 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;
> +
> +    /// Return the byte offset where the PCI Expansion ROM images begin in the GPU's ROM.
> +    ///
> +    /// The GPU's ROM may begin with an Init-from-ROM (IFR) header that precedes the PCI Expansion
> +    /// ROM images (VBIOS). When present, the PROM shadow method must parse this header to determine
> +    /// the offset where the PCI ROM images actually begin, and adjust all subsequent reads
> +    /// accordingly.
> +    ///
> +    /// On most GPUs this is not needed because the IFR microcode has already applied the ROM offset
> +    /// so that PROM reads transparently skip the header. On GA100, for some reason, the IFR offset
> +    /// is not applied to PROM reads. Therefore, the search for the PCI expansion must skip the IFR
> +    /// header, if found.
> +    fn rom_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {
> +        /// IFR signature.
> +        const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");
> +        /// ROM directory signature.
> +        const NV_ROM_DIRECTORY_IDENTIFIER: u32 = u32::from_le_bytes(*b"RFRD");
> +        /// Offset of the NV_PMGR_ROM_ADDR_OFFSET register in IFR Extended section.
> +        const IFR_SW_EXT_ROM_ADDR_OFFSET: usize = 4;
> +        /// Size of Redundant Firmware Flash Status section.
> +        const RFW_FLASH_STATUS_SIZE: usize = SZ_4K;
> +        /// Offset in the ROM Directory of the PCI Option ROM offset.
> +        const PCI_OPTION_ROM_OFFSET: usize = 8;
> +
> +        let signature = bar0.read(NV_PBUS_IFR_FMT_FIXED0).signature();
> +
> +        if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
> +            let fixed1 = bar0.read(NV_PBUS_IFR_FMT_FIXED1);
> +
> +            match fixed1.version() {
> +                1 | 2 => {
> +                    let fixed_data_size = usize::from(fixed1.fixed_data_size());
> +                    let pmgr_rom_addr_offset = fixed_data_size + IFR_SW_EXT_ROM_ADDR_OFFSET;
> +                    bar0.try_read32(Self::ROM_OFFSET + pmgr_rom_addr_offset)
> +                        .map(usize::from_safe_cast)
> +                }
> +                3 => {
> +                    let fixed2 = bar0.read(NV_PBUS_IFR_FMT_FIXED2);
> +                    let total_data_size = usize::from(fixed2.total_data_size());
> +                    let flash_status_offset =
> +                        usize::from_safe_cast(bar0.try_read32(Self::ROM_OFFSET + total_data_size)?);
> +                    let dir_offset = flash_status_offset + RFW_FLASH_STATUS_SIZE;
> +                    let dir_sig = bar0.try_read32(Self::ROM_OFFSET + dir_offset)?;
> +                    if dir_sig != NV_ROM_DIRECTORY_IDENTIFIER {
> +                        dev_err!(dev, "could not find IFR ROM directory\n");
> +                        return Err(EINVAL);
> +                    }
> +                    bar0.try_read32(Self::ROM_OFFSET + dir_offset + PCI_OPTION_ROM_OFFSET)
> +                        .map(usize::from_safe_cast)
> +                }
> +                _ => {
> +                    dev_err!(dev, "unsupported IFR header version {}\n", fixed1.version());
> +                    Err(EINVAL)
> +                }
> +            }
> +        } else {
> +            Ok(0)
> +        }
> +    }
> +
>      fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
>          Ok(Self {
>              dev,
>              bar0,
>              data: KVec::new(),
> -            current_offset: vbios_rom_offset(dev, bar0)?,
> +            current_offset: Self::rom_offset(dev, bar0)?,
>              last_found: false,
>          })
>      }
> @@ -186,7 +174,7 @@ fn read_more(&mut self, len: usize) -> Result {
>          let start = self.data.len();
>          let end = start + len;
>  
> -        if end > BIOS_MAX_SCAN_LEN {
> +        if end > Self::BIOS_MAX_SCAN_LEN {
>              dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
>              return Err(EINVAL);
>          }
> @@ -205,7 +193,7 @@ fn read_more(&mut self, len: usize) -> Result {
>          // Read ROM data bytes and push directly to `data`.
>          for addr in (start..end).step_by(core::mem::size_of::<u32>()) {
>              // Read 32-bit word from the VBIOS ROM
> -            let word = self.bar0.try_read32(ROM_OFFSET + addr)?;
> +            let word = self.bar0.try_read32(Self::ROM_OFFSET + addr)?;
>  
>              // Convert the `u32` to a 4 byte array and push each byte.
>              word.to_ne_bytes()
> @@ -267,7 +255,7 @@ fn next(&mut self) -> Option<Self::Item> {
>              return None;
>          }
>  
> -        if self.current_offset >= BIOS_MAX_SCAN_LEN {
> +        if self.current_offset >= Self::BIOS_MAX_SCAN_LEN {
>              dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
>              return None;
>          }
> @@ -275,7 +263,7 @@ fn next(&mut self) -> Option<Self::Item> {
>          // 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,
> +            Self::BIOS_READ_AHEAD_SIZE,
>              "parse initial BIOS image headers",
>          ) {
>              Ok(image) => image.image_size_bytes(),
> @@ -416,6 +404,9 @@ struct PcirStruct {
>  unsafe impl FromBytes for PcirStruct {}
>  
>  impl PcirStruct {
> +    /// The bit in `last_image` that indicates the last image.
> +    const LAST_IMAGE_BIT_MASK: u8 = 0x80;
> +
>      fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          let (pcir, _) = PcirStruct::from_bytes_copy_prefix(data).ok_or(EINVAL)?;
>  
> @@ -439,7 +430,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>  
>      /// Check if this is the last image in the ROM.
>      fn is_last(&self) -> bool {
> -        self.last_image & LAST_IMAGE_BIT_MASK != 0
> +        self.last_image & Self::LAST_IMAGE_BIT_MASK != 0
>      }
>  
>      /// Calculate image size in bytes from 512-byte blocks.
> @@ -505,10 +496,10 @@ struct BitToken {
>  // SAFETY: all bit patterns are valid for `BitToken`.
>  unsafe impl FromBytes for BitToken {}
>  
> -// Define the token ID for the Falcon data
> -const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
> -
>  impl BitToken {
> +    /// BIT token ID for Falcon data.
> +    const ID_FALCON_DATA: u8 = 0x70;
> +
>      /// Find a BIT token entry by BIT ID in a PciAtBiosImage
>      fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
>          let header = &image.bit_header;
> @@ -604,6 +595,9 @@ struct NpdeStruct {
>  unsafe impl FromBytes for NpdeStruct {}
>  
>  impl NpdeStruct {
> +    /// The bit in `last_image` that indicates the last image.
> +    const LAST_IMAGE_BIT_MASK: u8 = 0x80;
> +
>      fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
>          let (npde, _) = NpdeStruct::from_bytes_copy_prefix(data)?;
>  
> @@ -627,7 +621,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
>  
>      /// Check if this is the last image in the ROM.
>      fn is_last(&self) -> bool {
> -        self.last_image & LAST_IMAGE_BIT_MASK != 0
> +        self.last_image & Self::LAST_IMAGE_BIT_MASK != 0
>      }
>  
>      /// Calculate image size in bytes from 512-byte blocks.
> @@ -799,7 +793,7 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
>      /// between them, so subtract the PCI-AT image size here to convert it to a FWSEC-relative
>      /// offset.
>      fn falcon_data_offset(&self) -> Result<usize> {
> -        let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
> +        let token = self.get_bit_token(BitToken::ID_FALCON_DATA)?;
>          let offset = usize::from(token.data_offset);
>  
>          // Read the 4-byte falcon data pointer at the offset specified in the token.
> @@ -846,6 +840,17 @@ struct PmuLookupTableEntry {
>  // SAFETY: all bit patterns are valid for `PmuLookupTableEntry`.
>  unsafe impl FromBytes for PmuLookupTableEntry {}
>  
> +impl PmuLookupTableEntry {
> +    /// PMU lookup table application ID for firmware security license ucode.
> +    #[expect(dead_code)]
> +    const APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
> +    /// PMU lookup table application ID for debug FWSEC ucode.
> +    #[expect(dead_code)]
> +    const APPID_FWSEC_DBG: u8 = 0x45;
> +    /// PMU lookup table application ID for production FWSEC ucode.
> +    const APPID_FWSEC_PROD: u8 = 0x85;
> +}
> +
>  #[repr(C)]
>  struct PmuLookupTableHeader {
>      version: u8,
> @@ -923,7 +928,7 @@ fn new(
>          let pmu_lookup_table = PmuLookupTable::new(&second_fwsec.dev, pmu_lookup_data)?;
>  
>          let entry = pmu_lookup_table
> -            .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
> +            .find_entry_by_type(PmuLookupTableEntry::APPID_FWSEC_PROD)
>              .inspect_err(|e| {
>                  dev_err!(
>                      second_fwsec.dev,
>