Message-ID: <4fee85be-a8c5-4a99-8397-c93e79d72d15@nvidia.com>
Date: Tue, 20 May 2025 03:55:06 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>,
 Alexandre Courbot <acourbot@...dia.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Jonathan Corbet <corbet@....net>, John Hubbard <jhubbard@...dia.com>,
 Ben Skeggs <bskeggs@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 Alistair Popple <apopple@...dia.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, nouveau@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, Shirish Baskaran <sbaskaran@...dia.com>
Subject: Re: [PATCH v3 16/19] nova-core: Add support for VBIOS ucode
 extraction for boot

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@...dia.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@...dia.com: fix clippy warnings]
>> [acourbot@...dia.com: remove now-unneeded Devres acquisition]
>> [acourbot@...dia.com: fix read_more to read `len` bytes, not u32s]
>>
>> Cc: Alexandre Courbot <acourbot@...dia.com>
>> Cc: John Hubbard <jhubbard@...dia.com>
>> Cc: Shirish Baskaran <sbaskaran@...dia.com>
>> Cc: Alistair Popple <apopple@...dia.com>
>> Cc: Timur Tabi <ttabi@...dia.com>
>> Cc: Ben Skeggs <bskeggs@...dia.com>
>> Signed-off-by: Joel Fernandes <joelagnelf@...dia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.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

Powered by blists - more mailing lists