| 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
| ||
|
Message-ID: <bdb290d4-b369-4b8e-b78d-8c8d3cc07057@nvidia.com>
Date: Tue, 20 May 2025 11:11:12 -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:01 AM, Danilo Krummrich wrote:
> 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)?)
> }
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.
>>> 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.
thanks,
- Joel
Powered by blists - more mailing lists