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

[Joel Fernandes <joelagnelf@nvidia.com>]

> On May 20, 2025, at 11:37 AM, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Tue, May 20, 2025 at 11:11:12AM -0400, Joel Fernandes wrote:
>> On 5/20/2025 11:01 AM, Danilo Krummrich wrote:
>> 
>> I made this change and it LGTM. Thanks! I did not do the '.0' though since I
>> want to keep the readability, lets see in the next revision if that looks good.
> 
> I think readability, is just as good with `.0`, but I'm fine with either.

Cool.

> 
>>>>> 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 think this would be a fundamental rewrite of the patch. I am Ok with looking
>> into it as a future item, but right now I am not sure if it justifies not using
>> Option for these few. There's a lot of immediate work we have to do for boot,
>> lets please not block the patch on just this if that's Ok with you. If you want,
>> I could add a TODO here.
> 
> Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
> juggling fields and moving code around. The actual code should not change much.
> 
> Having Option<T> where the corresponding value T isn't actually optional is
> extremely confusing and makes it hard for everyone, but especially new
> contributors, to understand the code and can easily trick people into taking
> wrong assumptions.
> 
> Making the code reasonably accessible for (new) contributors is one of the
> objectives of nova and one of the learnings from nouveau.
> 
> Hence, let's get this right from the get-go please.

Ok, I will look into making this change. :-)

thanks,

 - Joel