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

On Wed, May 07, 2025 at 10:52:43PM +0900, Alexandre Courbot
wrote:
> From: Joel Fernandes <joelagnelf at 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 at nvidia.com: fix clippy warnings]
> [acourbot at nvidia.com: remove now-unneeded Devres acquisition]
> [acourbot at nvidia.com: fix read_more to read `len` bytes, not u32s]
> 
> Cc: Alexandre Courbot <acourbot at nvidia.com>
> Cc: John Hubbard <jhubbard at nvidia.com>
> Cc: Shirish Baskaran <sbaskaran at nvidia.com>
> Cc: Alistair Popple <apopple at nvidia.com>
> Cc: Timur Tabi <ttabi at nvidia.com>
> Cc: Ben Skeggs <bskeggs at nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot at 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 at 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 at nvidia.com: fix clippy warnings]
>> [acourbot at nvidia.com: remove now-unneeded Devres acquisition]
>> [acourbot at nvidia.com: fix read_more to read `len` bytes, not u32s]
>>
>> Cc: Alexandre Courbot <acourbot at nvidia.com>
>> Cc: John Hubbard <jhubbard at nvidia.com>
>> Cc: Shirish Baskaran <sbaskaran at nvidia.com>
>> Cc: Alistair Popple <apopple at nvidia.com>
>> Cc: Timur Tabi <ttabi at nvidia.com>
>> Cc: Ben Skeggs <bskeggs at nvidia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com>
>> Signed-off-by: Alexandre Courbot <acourbot at 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,
	})