Re: [PATCH v3 16/19] nova-core: Add support for VBIOS ucode extraction for boot

[Danilo Krummrich <dakr@kernel.org>]

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