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

[Joel Fernandes <joelagnelf@nvidia.com>]

Hello, Danilo,
Thanks for all the feedback. Due to the volume of feedback, I will respond
incrementally in multiple emails so we can discuss as we go - hope that's Ok but
let me know if that is annoying.

On 4/23/2025 10:06 AM, Danilo Krummrich wrote:
> On Sun, Apr 20, 2025 at 09:19:45PM +0900, Alexandre Courbot wrote:
>> From: Joel Fernandes <joelagnelf@nvidia.com>
>>
>> Add support for navigating and setting up vBIOS ucode data required for
[...]
>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..534107b708cab0eb8d0accf7daa5718edf030358
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/vbios.rs
>> @@ -0,0 +1,1103 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// To be removed when all code is used.
>> +#![allow(dead_code)]
> 
> Please not, use 'expect' and and only where needed. If it would be too much,
> it's probably a good indicator that we want to reduce the size of the patch for
> now.
> 

Sure, I will switch to expect. The addition of a bit of dead code is intentional
as we want to keep unused bits for future extension and lesser reader ambiguity.

Note that I've already been conservative with not adding too much more code than
we need (otherwise this patch could have been 2X the size), however VBIOS is a
complicated thing and I think we want to keep a little more than we need for
future extension for GPU families and proper documentation.

>> +
>> +//! VBIOS extraction and parsing.
>> +
>> +use crate::driver::Bar0;
>> +use crate::firmware::FalconUCodeDescV3;
>> +use core::convert::TryFrom;
>> +use kernel::devres::Devres;
>> +use kernel::error::Result;
>> +use kernel::prelude::*;
>> +
>> +/// The offset of the VBIOS ROM in the BAR0 space.
>> +const ROM_OFFSET: usize = 0x300000;
>> +/// The maximum length of the VBIOS ROM to scan into.
>> +const BIOS_MAX_SCAN_LEN: usize = 0x100000;
>> +/// The size to read ahead when parsing initial BIOS image headers.
>> +const BIOS_READ_AHEAD_SIZE: usize = 1024;
>> +
>> +// PMU lookup table entry types. Used to locate PMU table entries
>> +// in the Fwsec image, corresponding to falcon ucodes.
>> +#[allow(dead_code)]
>> +const FALCON_UCODE_ENTRY_APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
>> +#[allow(dead_code)]
>> +const FALCON_UCODE_ENTRY_APPID_FWSEC_DBG: u8 = 0x45;
>> +const FALCON_UCODE_ENTRY_APPID_FWSEC_PROD: u8 = 0x85;
>> +
>> +pub(crate) struct Vbios {
>> +    pub fwsec_image: Option<FwSecBiosImage>,
>> +}
>> +
>> +impl Vbios {
>> +    /// Read bytes from the ROM at the current end of the data vector
>> +    fn read_more(bar0: &Devres<Bar0>, data: &mut KVec<u8>, len: usize) -> Result {
>> +        let current_len = data.len();
>> +        let start = ROM_OFFSET + current_len;
>> +
>> +        // Ensure length is a multiple of 4 for 32-bit reads
>> +        if len % core::mem::size_of::<u32>() != 0 {
>> +            pr_err!("VBIOS read length {} is not a multiple of 4\n", len);
> 
> Please don't use any of the pr_*() print macros within a driver, use the dev_*()
> ones instead.

Ok I'll switch to this. One slight complication is I've to retrieve the 'dev'
from the Bar0 and pass that along, but that should be doable.

> 
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // Allocate and zero-initialize the required memory
> 
> That's obvious from the code, if you feel this needs a comment, better explain
> what we need it for, why zero-initialize, etc.

Sure, actually the extends_with() is a performance optimization as we want to do
only a single allocation and then fill in the allocated data. It has nothing to
do with 0-initializing per-se. I will adjust the comment, but:

This code...

>> +        data.extend_with(len, 0, GFP_KERNEL)?;
>> +        with_bar!(?bar0, |bar0_ref| {
>> +            let dst = &mut data[current_len..current_len + len];
>> +            for (idx, chunk) in dst
>> +                .chunks_exact_mut(core::mem::size_of::<u32>())
>> +                .enumerate()
>> +            {
>> +                let addr = start + (idx * core::mem::size_of::<u32>());
>> +                // Convert the u32 to a 4 byte array. We use the .to_ne_bytes()
>> +                // method out of convenience to convert the 32-bit integer as it
>> +                // is in memory into a byte array without any endianness
>> +                // conversion or byte-swapping.
>> +                chunk.copy_from_slice(&bar0_ref.try_read32(addr)?.to_ne_bytes());
>> +            }
>> +            Ok(())
>> +        })?;
>> +
>> +        Ok(())
>> +    }
..actually initially was:

+        with_bar!(self.bar0, |bar0| {
+            // Get current length
+            let current_len = self.data.len();
+
+            // Read ROM data bytes push directly to vector
+            for i in 0..bytes as usize {
+                // Read byte from the VBIOS ROM and push it to the data vector
+                let rom_addr = ROM_OFFSET + current_len + i;
+                let byte = bar0.try_readb(rom_addr)?;
+                self.data.push(byte, GFP_KERNEL)?;

Where this bit could result in a lot of allocation.

There was an unsafe() way of not having to do this but we settled with
extends_with().

Thoughts?

Thanks.