Re: [PATCH v7 07/31] gpu: nova-core: move firmware image parsing code to firmware.rs

[John Hubbard]

On 3/23/26 6:19 AM, Alexandre Courbot wrote:
> On Wed Mar 18, 2026 at 7:53 AM JST, John Hubbard wrote:
>> Up until now, only the GSP required parsing of its firmware headers.
>> However, upcoming support for Hopper/Blackwell+ adds another firmware
>> image (FMC), along with another format (ELF32).
>>
>> Therefore, the current ELF64 section parsing support needs to be moved
>> up a level, so that both of the above can use it.
>>
>> There are no functional changes. This is pure code movement.
>>
>> Reviewed-by: Gary Guo <gary@xxxxxxxxxxx>
>> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/firmware.rs     | 88 +++++++++++++++++++++++++
>>  drivers/gpu/nova-core/firmware/gsp.rs | 93 ++-------------------------
>>  2 files changed, 94 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> index 2bb20081befd..177b8ede151c 100644
>> --- a/drivers/gpu/nova-core/firmware.rs
>> +++ b/drivers/gpu/nova-core/firmware.rs
>> @@ -457,3 +457,91 @@ pub(crate) const fn create(
>>          this.0
>>      }
>>  }
>> +
>> +/// Ad-hoc and temporary module to extract sections from ELF images.
>> +///
>> +/// Some firmware images are currently packaged as ELF files, where sections names are used as keys
>> +/// to specific and related bits of data. Future firmware versions are scheduled to move away from
>> +/// that scheme before nova-core becomes stable, which means this module will eventually be
>> +/// removed.
>> +mod elf {
>> +    use core::mem::size_of;
> 
> This import is not needed, `size_of` is already in the prelude.


The `mod elf` block is a nested module that doesn't have `use
kernel::prelude::*;` in scope, so `size_of` isn't available without the
explicit import.

Or some it seems. I'm not experienced with the module scoping game,
and so I may have it wrong.

> 
>> +
>> +    use kernel::{
>> +        bindings,
>> +        str::CStr,
>> +        transmute::FromBytes, //
>> +    };
>> +
>> +    /// Newtype to provide a [`FromBytes`] implementation.
>> +    #[repr(transparent)]
>> +    struct Elf64Hdr(bindings::elf64_hdr);
>> +    // SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
>> +    unsafe impl FromBytes for Elf64Hdr {}
>> +
>> +    #[repr(transparent)]
>> +    struct Elf64SHdr(bindings::elf64_shdr);
>> +    // SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
>> +    unsafe impl FromBytes for Elf64SHdr {}
>> +
>> +    /// Tries to extract section with name `name` from the ELF64 image `elf`, and returns it.
>> +    pub(super) fn elf64_section<'a, 'b>(elf: &'a [u8], name: &'b str) -> Option<&'a [u8]> {
>> +        let hdr = &elf
>> +            .get(0..size_of::<bindings::elf64_hdr>())
>> +            .and_then(Elf64Hdr::from_bytes)?
>> +            .0;
>> +
>> +        // Get all the section headers.
>> +        let mut shdr = {
>> +            let shdr_num = usize::from(hdr.e_shnum);
>> +            let shdr_start = usize::try_from(hdr.e_shoff).ok()?;
>> +            let shdr_end = shdr_num
>> +                .checked_mul(size_of::<Elf64SHdr>())
>> +                .and_then(|v| v.checked_add(shdr_start))?;
>> +
>> +            elf.get(shdr_start..shdr_end)
>> +                .map(|slice| slice.chunks_exact(size_of::<Elf64SHdr>()))?
>> +        };
>> +
>> +        // Get the strings table.
>> +        let strhdr = shdr
>> +            .clone()
>> +            .nth(usize::from(hdr.e_shstrndx))
>> +            .and_then(Elf64SHdr::from_bytes)?;
>> +
>> +        // Find the section which name matches `name` and return it.
>> +        shdr.find(|&sh| {
>> +            let Some(hdr) = Elf64SHdr::from_bytes(sh) else {
>> +                return false;
>> +            };
>> +
>> +            let Some(name_idx) = strhdr
>> +                .0
>> +                .sh_offset
>> +                .checked_add(u64::from(hdr.0.sh_name))
>> +                .and_then(|idx| usize::try_from(idx).ok())
>> +            else {
>> +                return false;
>> +            };
>> +
>> +            // Get the start of the name.
>> +            elf.get(name_idx..)
>> +                .and_then(|nstr| CStr::from_bytes_until_nul(nstr).ok())
>> +                // Convert into str.
>> +                .and_then(|c_str| c_str.to_str().ok())
>> +                // Check that the name matches.
>> +                .map(|str| str == name)
>> +                .unwrap_or(false)
>> +        })
>> +        // Return the slice containing the section.
>> +        .and_then(|sh| {
>> +            let hdr = Elf64SHdr::from_bytes(sh)?;
>> +            let start = usize::try_from(hdr.0.sh_offset).ok()?;
>> +            let end = usize::try_from(hdr.0.sh_size)
>> +                .ok()
>> +                .and_then(|sh_size| start.checked_add(sh_size))?;
>> +
>> +            elf.get(start..end)
>> +        })
>> +    }
>> +}
>> diff --git a/drivers/gpu/nova-core/firmware/gsp.rs b/drivers/gpu/nova-core/firmware/gsp.rs
>> index 8bbc3809c640..c6e71339b28e 100644
>> --- a/drivers/gpu/nova-core/firmware/gsp.rs
>> +++ b/drivers/gpu/nova-core/firmware/gsp.rs
>> @@ -1,5 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  
>> +use core::mem::size_of_val;
> 
> And this one is unneeded as well. Actually I mentioned that in my v6
> review [1].

Same situation: firmware/gsp.rs doesn't import the kernel prelude, so
`size_of_val` needs the explicit import.

> 
> [1] https://lore.kernel.org/all/DGZ150DHI878.2YXL15FY7W0GG@xxxxxxxxxx/
> 

thanks,
-- 
John Hubbard