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:32 PM, Dave Airlie wrote:
> On Wed, 21 May 2025 at 04:13, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>>
>>
>> On 5/20/2025 11:36 AM, Danilo Krummrich wrote:
>>>>> 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.
> 
> I just want to back Danilo up on this concept as well.
> 
> When I did the experiments code, I faced the not fully constructed
> object problem a lot, and I tried to resist the C pattern of Option<>
> all the things, it's a very C based thing where we create an object
> then initialise it as we go, and it's not a great pattern to have for
> rust code.
> 
> I'm not a huge fan of constructor/builder objects either if they can
> be avoided, please do, and I tried to also avoid proliferating them,
> but I think for most things we can build the pieces and then the final
> object as we go, it just requires doing so from the start, and not
> giving into the Option<> pattern.

Sure, I am on the same page there as well. For next revision of this patch,
struct Vbios will contain a struct FwsecBiosImage without any Option in either
struct Vbios or struct FwsecBiosImage. This is only logical, because there is
nothing optional about it (in what Vbios::new() returns).

thanks,

 - Joel