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

[Joel Fernandes <joelagnelf@nvidia.com>]

On 5/20/2025 5:30 AM, Danilo Krummrich wrote:
> 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.
> :-)

Point taken and fixed! ;-)

>>>
>>>> +                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.

But your code below does Vbios(second) where Vbios is an 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)

I can't do that become second is a mutable reference in the above snippet.

But this works:
             Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })

(This did require changing 'Some(second)' to 'Some(second_ref)', see below.)

>>> 	} 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.

Sure, ok, yeah I made this change in another thread we are discussing so we are
good.

So the code here now looks like the below, definitely better, thanks! :

            if let (Some(second_ref), Some(first), Some(pci_at)) =
                (second.as_mut(), first_fwsec_image, pci_at_image)
            {
                second_ref
                    .setup_falcon_data(pdev, &pci_at, &first)
                    .inspect_err(|e| {
                        dev_err!(..)
                    })?;
                Ok(Vbios { fwsec_image: second.take().ok_or(EINVAL)? })
            } else {
                dev_err!(
                    pdev.as_ref(),
                    "Missing required images for falcon data setup, skipping\n"
                );
                Err(EINVAL)
            }

>>>> +
>>>> +        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 looked into it, I could not git rid of those ones because we need to
initialize in the "impl TryFrom<BiosImageBase> for BiosImage {"

            0xE0 => Ok(BiosImage::FwSec(FwSecBiosImage {
                base,
                falcon_data_offset: None,
                pmu_lookup_table: None,
                falcon_ucode_offset: None,
            })),

And these fields will not be determined until much later, because as is the case
with the earlier example, these fields cannot be determined until all the images
are parsed.

> 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.

Understood, thanks.

>> 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,
> 	})

Yes, I made the change during our review on the other thread and will be posted
in the next posting. Sorry for any confusion.

thanks,

 - Joel