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

[Danilo Krummrich <dakr@kernel.org>]

On Tue, May 20, 2025 at 09:43:42AM -0400, Joel Fernandes wrote:
> 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:
> 
> 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)
>             }

Sorry, my code-snipped was incorrect indeed. Let me paste what I actually
intended (and this time properly compile checked) and should be even better:

	if let (Some(mut second), Some(first), Some(pci_at)) =
	    (second_fwsec_image, 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)
	}

So, with this second is the actual value and not just a reference. :)

And the methods can become:

	pub(crate) fn fwsec_header(&self, pdev: &device::Device) -> Result<&FalconUCodeDescV3> {
	    self.0.fwsec_header(pdev)
	}
	
	pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) -> Result<&[u8]> {
	    self.0.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
	}
	
	pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) -> Result<&[u8]> {
	    self.0.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
	}

However, I don't understand why they're not just implemented for FwSecBiosImage
itself this way. You can just implement Deref for Vbios then.

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

You should not use TryFrom, but instead use a normal constructor, such as

	BiosImage::new(base_bios_image)

and do the parsing within this constructor.

If you want a helper type with Options while parsing that's totally fine, but
the final result can clearly be without Options. For instance:

	struct Data {
	   image: KVec<u8>,
	}

	impl Data {
	   fn new() -> Result<Self> {
	      let parser = DataParser::new();

	      Self { image: parser.parse()? }
	   }

	   fn load_image(&self) {
	      ...
	   }
	}

	struct DataParser {
	   // Only some images have a checksum.
	   checksum: Option<u64>,
	   // Some images have an extra offset.
	   offset: Option<u64>,
	   // Some images need to be patched.
	   patch: Option<KVec<u8>>,
	   image: KVec<u8>,
	}

	impl DataParser {
	   fn new() -> Self {
	      Self {
	         checksum: None,
	         offset: None,
	         patch: None,
	         bytes: KVec::new(),
	      }
	   }

	   fn parse(self) -> Result<KVec<u8>> {
	      // Fetch all the required data.
	      self.fetch_checksum()?;
	      self.fetch_offset()?;
	      self.fetch_patch()?;
	      self.fetch_byes()?;

	      // Doesn't do anything if `checksum == None`.
	      self.validate_checksum()?;

	      // Doesn't do anything if `offset == None`.
	      self.apply_offset()?;

	      // Doesn't do anything if `patch == None`.
	      self.apply_patch()?;

	      // Return the final image.
	      self.image
	   }
	}

I think the pattern here is the same, but in this example you keep working with
the DataParser, instead of a new instance of Data.

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