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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dcd249d6-7e99-476e-b216-8ca9e1a936e5@nvidia.com>
Date: Tue, 20 May 2025 14:13:02 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Alexandre Courbot <acourbot@...dia.com>, 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



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 implemented the Data parsing pattern like the following, the final
FwSecBiosImage will not have optional fields as you suggested. It does get rid
of 2 additional fields as well which are not needed after vbios parsing completes.

https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=nova/vbios&id=8cc852fe5573890596a91a2a935b3af24dcb9f04

Hope that looks Ok now! I am open to naming FwSecBiosPartial as FwSecBiosData if
that's better.

The full file after the change:
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/tree/drivers/gpu/nova-core/vbios.rs?h=nova/vbios&id=8cc852fe5573890596a91a2a935b3af24dcb9f04

thanks,

 - Joel