[PATCH] rust: add basic ELF sections parser

[PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

Add a simple ELF sections parser for unpacking loaded binaries from
user-space. This is not intended to become a fully-fledged ELF parser,
just a helper to parse firmwares packaged in that format.

This parser is notably helpful for NVIDIA's GSP firmware, which is
provided as an ELF binary using sections to separate the firmware code
to its other components like chipset-specific signatures.

Since the data source is likely to be user-space, checked arithmetic
operations and strict bound checking are used.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
This will soon be needed in order to load the GSP firmware in nova-core,
so sending this early for separate review.
---
 rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs |   1 +
 2 files changed, 323 insertions(+)

diff --git a/rust/kernel/elf.rs b/rust/kernel/elf.rs
new file mode 100644
index 0000000000000000000000000000000000000000..c6af2f23a360a76992546532214fdc093522c797
--- /dev/null
+++ b/rust/kernel/elf.rs
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Simple ELF parser, useful for e.g. extracting relevant sections from loaded firmware.
+//!
+//! C headers: [`include/uapi/linux/elf.h`](srctree/include/uapi/linux/elf.h)
+
+use core::ops::Deref;
+
+use crate::bindings;
+use crate::prelude::*;
+use crate::transmute::FromBytes;
+
+/// Class of the ELF binary, i.e. 32 or 64 bits.
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
+#[repr(u32)]
+enum ElfClass {
+    Class32 = bindings::ELFCLASS32,
+    Class64 = bindings::ELFCLASS64,
+}
+
+impl ElfClass {
+    /// Validate and convert an `ELFCLASS*` u8 value.
+    fn from_u8(value: u8) -> Option<Self> {
+        match value as u32 {
+            bindings::ELFCLASS32 => Some(Self::Class32),
+            bindings::ELFCLASS64 => Some(Self::Class64),
+            _ => None,
+        }
+    }
+}
+
+/// ELF magic header.
+const ELF_MAGIC: [u8; 4] = [
+    bindings::ELFMAG0 as u8,
+    bindings::ELFMAG1,
+    bindings::ELFMAG2,
+    bindings::ELFMAG3,
+];
+
+/// Wraps the passed `inner` type into an `outer` newtype structure that implements [`FromBytes`]
+/// and derefs into its inner type.
+///
+/// This is intended for local use with ELF structures for which any byte stream is valid.
+///
+/// # Safety
+///
+/// `inner` must be a type that would implement [`FromBytes`] if the implementation rules allowed
+/// us to.
+///
+/// TODO: replace with FromBytes' own transmute method once it is available.
+macro_rules! frombytes_wrapper {
+    ($(struct $outer:ident{$inner:ty};)*) => {
+    $(
+        #[repr(transparent)]
+        #[derive(Clone, Copy)]
+        struct $outer($inner);
+        /// SAFETY: any set of values is a valid representation for this type.
+        unsafe impl FromBytes for $outer {}
+        impl Deref for $outer {
+            type Target = $inner;
+
+            fn deref(&self) -> &Self::Target {
+                &self.0
+            }
+        }
+    )*
+    };
+}
+
+frombytes_wrapper!(
+    struct Elf32Ehdr { bindings::Elf32_Ehdr };
+    struct Elf64Ehdr { bindings::Elf64_Ehdr };
+    struct Elf32Shdr { bindings::Elf32_Shdr };
+    struct Elf64Shdr { bindings::Elf64_Shdr };
+);
+
+/// Reinterprets a byte slice as a structure `T` and return a copy after validating its length.
+///
+/// TODO: replace with FromBytes' own transmute method once it is available.
+fn read_from_bytes<T: Copy + FromBytes>(data: &[u8]) -> Result<T> {
+    if core::mem::size_of::<T>() > data.len() {
+        Err(EINVAL)
+    } else {
+        // SAFETY: `data` is valid for reads and long enough to contain an instance of `T`.
+        Ok(unsafe { core::ptr::read_unaligned(data.as_ptr() as *const T) })
+    }
+}
+
+/// Converts a 32-bit section header to its 64-bit equivalent.
+impl From<Elf32Shdr> for Elf64Shdr {
+    fn from(shdr32: Elf32Shdr) -> Self {
+        Elf64Shdr(bindings::Elf64_Shdr {
+            sh_name: shdr32.sh_name,
+            sh_type: shdr32.sh_type,
+            sh_flags: shdr32.sh_flags as _,
+            sh_addr: shdr32.sh_addr as _,
+            sh_offset: shdr32.sh_offset as _,
+            sh_size: shdr32.sh_size as _,
+            sh_link: shdr32.sh_link,
+            sh_info: shdr32.sh_info,
+            sh_addralign: shdr32.sh_addralign as _,
+            sh_entsize: shdr32.sh_entsize as _,
+        })
+    }
+}
+
+/// Safely obtain a sub-slice from `data`.
+fn get_slice(data: &[u8], offset: usize, size: usize) -> Result<&[u8]> {
+    offset
+        .checked_add(size)
+        .and_then(|end| data.get(offset..end))
+        .ok_or(EOVERFLOW)
+}
+
+/// A sections Parser for an ELF binary presented as a bytes slice.
+///
+/// # Examples
+///
+/// ```no_run
+/// # fn no_run() -> Result<(), Error> {
+/// # let fw: [u8; 0] = [];
+/// use kernel::elf;
+///
+/// // Obtain the data from the `.fwimage` section:
+/// let parser = elf::Parser::new(&fw)?;
+/// let fwimage = parser.sections_iter()?
+///     .filter_map(Result::ok)
+///     .find(|section| section.name == ".fwimage")
+///     .map(|section| section.data)
+///     .ok_or(EINVAL)?;
+///
+/// # Ok(())
+/// # }
+/// ```
+pub struct Parser<'a> {
+    /// Content of the ELF binary.
+    data: &'a [u8],
+    /// Class of the ELF data (32 or 64 bits).
+    class: ElfClass,
+    /// Offset of the section header table.
+    shoff: u64,
+    /// Size in bytes of a section header table entry.
+    shentsize: u16,
+    /// Number of entries in the section header table.
+    shnum: u16,
+    /// Section header table index of the entry containing the section name string table.
+    shstrndx: u16,
+}
+
+impl<'a> Parser<'a> {
+    /// Creates a new parser from a bytes array containing an ELF file.
+    pub fn new(data: &'a [u8]) -> Result<Self> {
+        // Validate ELF magic number and class.
+        let class = data
+            .get(0..bindings::EI_NIDENT as usize)
+            .filter(|ident| {
+                ident[bindings::EI_MAG0 as usize..=bindings::EI_MAG3 as usize] == ELF_MAGIC
+            })
+            .and_then(|ident| ElfClass::from_u8(ident[bindings::EI_CLASS as usize]))
+            .ok_or(EINVAL)?;
+
+        // Read the appropriate ELF header (32 or 64 bit).
+        let (shoff, shnum, shentsize, shstrndx) = match class {
+            ElfClass::Class64 => {
+                let header = read_from_bytes::<Elf64Ehdr>(data)?;
+                (
+                    header.e_shoff,
+                    header.e_shnum,
+                    header.e_shentsize,
+                    header.e_shstrndx,
+                )
+            }
+            ElfClass::Class32 => {
+                let header = read_from_bytes::<Elf32Ehdr>(data)?;
+                (
+                    header.e_shoff as u64,
+                    header.e_shnum,
+                    header.e_shentsize,
+                    header.e_shstrndx,
+                )
+            }
+        };
+
+        Ok(Self {
+            data,
+            class,
+            shoff,
+            shentsize,
+            shnum,
+            shstrndx,
+        })
+    }
+
+    /// Returns the section header at `index`, normalized as a 64-bit header.
+    fn section_header(&self, index: u16) -> Result<Elf64Shdr> {
+        if index >= self.shnum {
+            return Err(EINVAL);
+        }
+
+        let offset = (index as u64)
+            .checked_mul(self.shentsize as u64)
+            .and_then(|r| r.checked_add(self.shoff))
+            .ok_or(EOVERFLOW)
+            .and_then(|offset| usize::try_from(offset).map_err(|_| EOVERFLOW))?;
+
+        let header_slice = self.data.get(offset..).ok_or(EINVAL)?;
+
+        match self.class {
+            ElfClass::Class64 => read_from_bytes::<Elf64Shdr>(header_slice),
+            ElfClass::Class32 => read_from_bytes::<Elf32Shdr>(header_slice).map(Into::into),
+        }
+    }
+
+    /// Retrieves the raw byte data of the section header string table.
+    fn string_table_data(&self) -> Result<&'a [u8]> {
+        let strtab_header = self.section_header(self.shstrndx)?;
+        if strtab_header.sh_type != bindings::SHT_STRTAB {
+            return Err(EINVAL);
+        }
+
+        let offset = usize::try_from(strtab_header.sh_offset).map_err(|_| EOVERFLOW)?;
+        let size = usize::try_from(strtab_header.sh_size).map_err(|_| EOVERFLOW)?;
+        get_slice(self.data, offset, size)
+    }
+
+    /// Looks up a section name in the string table.
+    fn section_name(&self, strtab: &'a [u8], name_offset: u32) -> Result<&'a str> {
+        let name_bytes_with_suffix = strtab.get(name_offset as usize..).ok_or(EINVAL)?;
+        let name_bytes = name_bytes_with_suffix
+            .split(|&c| c == 0)
+            .next()
+            .unwrap_or(&[]);
+        core::str::from_utf8(name_bytes).map_err(|_| EINVAL)
+    }
+
+    /// Returns an iterator over the sections.
+    pub fn sections_iter(&'a self) -> Result<SectionsIterator<'a>> {
+        let strtab_data = self.string_table_data()?;
+        Ok(SectionsIterator {
+            parser: self,
+            strtab_data,
+            current_index: 0,
+        })
+    }
+}
+
+/// Describes a single ELF section.
+pub struct Section<'a> {
+    /// Name of the section.
+    pub name: &'a str,
+    /// Type of the section (e.g., `SHT_PROGBITS`, `SHT_STRTAB`).
+    pub type_: u32,
+    /// Section flags (e.g., `SHF_ALLOC`, `SHF_EXECINSTR`).
+    pub flags: u64,
+    /// Virtual address of the section in memory.
+    pub addr: u64,
+    /// Byte slice containing the raw data of the section from the file.
+    pub data: &'a [u8],
+}
+
+/// An iterator over the sections of an ELF file.
+///
+/// Note that the [`Iterator::next`] method returns a [`Result`]. This is because a section header
+/// could be invalid, but the user still want to keep parsing the next sections.
+pub struct SectionsIterator<'a> {
+    parser: &'a Parser<'a>,
+    /// Slice to the string table data.
+    strtab_data: &'a [u8],
+    /// Index of the next section to be returned by the iterator.
+    current_index: u16,
+}
+
+impl<'a> Iterator for SectionsIterator<'a> {
+    type Item = Result<Section<'a>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.current_index >= self.parser.shnum {
+            return None;
+        }
+
+        let index = self.current_index;
+        // This cannot overflow because we would have returned if `current_index` was equal to
+        // `shnum`, which is a representable `u16`,
+        self.current_index += 1;
+
+        // Skip the NULL section header (index 0).
+        if index == 0 {
+            return self.next();
+        }
+
+        let header = match self.parser.section_header(index) {
+            Ok(header) => header,
+            Err(e) => return Some(Err(e)),
+        };
+
+        let section_name = match self.parser.section_name(self.strtab_data, header.sh_name) {
+            Ok(name) => name,
+            Err(e) => return Some(Err(e)),
+        };
+
+        let section_data = if header.sh_type == bindings::SHT_NOBITS {
+            &self.parser.data[0..0]
+        } else {
+            match get_slice(
+                self.parser.data,
+                header.sh_offset as usize,
+                header.sh_size as usize,
+            ) {
+                Ok(slice) => slice,
+                Err(e) => return Some(Err(e)),
+            }
+        };
+
+        Some(Ok(Section {
+            name: section_name,
+            type_: header.sh_type,
+            flags: header.sh_flags,
+            addr: header.sh_addr,
+            data: section_data,
+        }))
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ab0286857061d2de1be0279cbd2cd3490e5a48c3..5a9a780073b23d46e1ca70dd944826ee1c902bca 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -52,6 +52,7 @@
 pub mod driver;
 #[cfg(CONFIG_DRM = "y")]
 pub mod drm;
+pub mod elf;
 pub mod error;
 pub mod faux;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]

---
base-commit: 61479ae38cb7bf6083de302598b7d491ec54168a
change-id: 20250514-elf-b9b16f56dd89

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Thu, May 15, 2025 at 03:03:51PM +0900, Alexandre Courbot wrote:
> Add a simple ELF sections parser for unpacking loaded binaries from
> user-space. This is not intended to become a fully-fledged ELF parser,
> just a helper to parse firmwares packaged in that format.
> 
> This parser is notably helpful for NVIDIA's GSP firmware, which is
> provided as an ELF binary using sections to separate the firmware code
> to its other components like chipset-specific signatures.
> 
> Since the data source is likely to be user-space, checked arithmetic
> operations and strict bound checking are used.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> This will soon be needed in order to load the GSP firmware in nova-core,
> so sending this early for separate review.
> ---
>  rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++

Why is this not just done in userspace and then have userspace feed the
proper elf sections to the kernel through the firmware interface?
Having to parse elf seems crazy for the kernel to be forced to do here
as the kernel should NOT be touching anything in a firmware blob other
than passing it off to the firmware directly.

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

Hi Greg,

On Thu May 15, 2025 at 4:38 PM JST, Greg KH wrote:
> On Thu, May 15, 2025 at 03:03:51PM +0900, Alexandre Courbot wrote:
>> Add a simple ELF sections parser for unpacking loaded binaries from
>> user-space. This is not intended to become a fully-fledged ELF parser,
>> just a helper to parse firmwares packaged in that format.
>> 
>> This parser is notably helpful for NVIDIA's GSP firmware, which is
>> provided as an ELF binary using sections to separate the firmware code
>> to its other components like chipset-specific signatures.
>> 
>> Since the data source is likely to be user-space, checked arithmetic
>> operations and strict bound checking are used.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> This will soon be needed in order to load the GSP firmware in nova-core,
>> so sending this early for separate review.
>> ---
>>  rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> Why is this not just done in userspace and then have userspace feed the
> proper elf sections to the kernel through the firmware interface?
> Having to parse elf seems crazy for the kernel to be forced to do here
> as the kernel should NOT be touching anything in a firmware blob other
> than passing it off to the firmware directly.

FWIW, the GSP firmware in question is already in linux-firmware and
loaded by e.g. Nouveau.

I am not sure how userspace could feed the proper ELF sections otherwise
than by splitting the ELF binary into as many files as there are
sections. Is that what you imply, or is there another means that would
preserve the current firmware format?

Note also that in this particular case, the kernel cannot just pass the
firmware without modifying it anyway since the signatures relevant to
the chipset need to be patched into the right place before it is loaded.

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Thu May 15, 2025 at 5:32 PM JST, Alexandre Courbot wrote:
> Hi Greg,
>
> On Thu May 15, 2025 at 4:38 PM JST, Greg KH wrote:
>> On Thu, May 15, 2025 at 03:03:51PM +0900, Alexandre Courbot wrote:
>>> Add a simple ELF sections parser for unpacking loaded binaries from
>>> user-space. This is not intended to become a fully-fledged ELF parser,
>>> just a helper to parse firmwares packaged in that format.
>>> 
>>> This parser is notably helpful for NVIDIA's GSP firmware, which is
>>> provided as an ELF binary using sections to separate the firmware code
>>> to its other components like chipset-specific signatures.
>>> 
>>> Since the data source is likely to be user-space, checked arithmetic
>>> operations and strict bound checking are used.
>>> 
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>> ---
>>> This will soon be needed in order to load the GSP firmware in nova-core,
>>> so sending this early for separate review.
>>> ---
>>>  rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> Why is this not just done in userspace and then have userspace feed the
>> proper elf sections to the kernel through the firmware interface?
>> Having to parse elf seems crazy for the kernel to be forced to do here
>> as the kernel should NOT be touching anything in a firmware blob other
>> than passing it off to the firmware directly.
>
> FWIW, the GSP firmware in question is already in linux-firmware and
> loaded by e.g. Nouveau.
>
> I am not sure how userspace could feed the proper ELF sections otherwise
> than by splitting the ELF binary into as many files as there are
> sections. Is that what you imply, or is there another means that would
> preserve the current firmware format?
>
> Note also that in this particular case, the kernel cannot just pass the
> firmware without modifying it anyway since the signatures relevant to
> the chipset need to be patched into the right place before it is loaded.

Quick nit, as that last statement was not entirely correct: while we do
patch some loaded firmware with signatures, this is not the case for the
GSP (the one in ELF format). Not that it changes the point, but for the
sake of accuracy. :)

The point being that even without using ELF as a container format, we do
need to parse header structures in loaded firmware files anyway, so the
kernel cannot simply act as a dumb pipe for firmware. And since we need
to add structure, let's at least use a format that is simple, well
accepted and which layout is already in the kernel.

Or if ELF is the problem, I don't mind introducing a WAD loader. ;)

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Thu, May 15, 2025 at 08:25:33PM +0900, Alexandre Courbot wrote:
> On Thu May 15, 2025 at 5:32 PM JST, Alexandre Courbot wrote:
> > Hi Greg,
> >
> > On Thu May 15, 2025 at 4:38 PM JST, Greg KH wrote:
> >> On Thu, May 15, 2025 at 03:03:51PM +0900, Alexandre Courbot wrote:
> >>> Add a simple ELF sections parser for unpacking loaded binaries from
> >>> user-space. This is not intended to become a fully-fledged ELF parser,
> >>> just a helper to parse firmwares packaged in that format.
> >>> 
> >>> This parser is notably helpful for NVIDIA's GSP firmware, which is
> >>> provided as an ELF binary using sections to separate the firmware code
> >>> to its other components like chipset-specific signatures.
> >>> 
> >>> Since the data source is likely to be user-space, checked arithmetic
> >>> operations and strict bound checking are used.
> >>> 
> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >>> ---
> >>> This will soon be needed in order to load the GSP firmware in nova-core,
> >>> so sending this early for separate review.
> >>> ---
> >>>  rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>
> >> Why is this not just done in userspace and then have userspace feed the
> >> proper elf sections to the kernel through the firmware interface?
> >> Having to parse elf seems crazy for the kernel to be forced to do here
> >> as the kernel should NOT be touching anything in a firmware blob other
> >> than passing it off to the firmware directly.
> >
> > FWIW, the GSP firmware in question is already in linux-firmware and
> > loaded by e.g. Nouveau.
> >
> > I am not sure how userspace could feed the proper ELF sections otherwise
> > than by splitting the ELF binary into as many files as there are
> > sections. Is that what you imply, or is there another means that would
> > preserve the current firmware format?
> >
> > Note also that in this particular case, the kernel cannot just pass the
> > firmware without modifying it anyway since the signatures relevant to
> > the chipset need to be patched into the right place before it is loaded.
> 
> Quick nit, as that last statement was not entirely correct: while we do
> patch some loaded firmware with signatures, this is not the case for the
> GSP (the one in ELF format). Not that it changes the point, but for the
> sake of accuracy. :)
> 
> The point being that even without using ELF as a container format, we do
> need to parse header structures in loaded firmware files anyway, so the
> kernel cannot simply act as a dumb pipe for firmware. And since we need
> to add structure, let's at least use a format that is simple, well
> accepted and which layout is already in the kernel.
> 
> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)

The "problem" I'm not understanding is why does the kernel have to do
any of this parsing at all?  What does it do with these segments that
userspace can't do instead?  Why does patching have to be done within
the kernel at all?  What prevents all of this from being done elsewhere?

And ELF parsing is "tricky" in places, and you aren't using the existing
elf parser, as proof of needing a new parser in rust :)

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Thu May 15, 2025 at 8:42 PM JST, Greg KH wrote:
> On Thu, May 15, 2025 at 08:25:33PM +0900, Alexandre Courbot wrote:
>> On Thu May 15, 2025 at 5:32 PM JST, Alexandre Courbot wrote:
>> > Hi Greg,
>> >
>> > On Thu May 15, 2025 at 4:38 PM JST, Greg KH wrote:
>> >> On Thu, May 15, 2025 at 03:03:51PM +0900, Alexandre Courbot wrote:
>> >>> Add a simple ELF sections parser for unpacking loaded binaries from
>> >>> user-space. This is not intended to become a fully-fledged ELF parser,
>> >>> just a helper to parse firmwares packaged in that format.
>> >>> 
>> >>> This parser is notably helpful for NVIDIA's GSP firmware, which is
>> >>> provided as an ELF binary using sections to separate the firmware code
>> >>> to its other components like chipset-specific signatures.
>> >>> 
>> >>> Since the data source is likely to be user-space, checked arithmetic
>> >>> operations and strict bound checking are used.
>> >>> 
>> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> >>> ---
>> >>> This will soon be needed in order to load the GSP firmware in nova-core,
>> >>> so sending this early for separate review.
>> >>> ---
>> >>>  rust/kernel/elf.rs | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >>
>> >> Why is this not just done in userspace and then have userspace feed the
>> >> proper elf sections to the kernel through the firmware interface?
>> >> Having to parse elf seems crazy for the kernel to be forced to do here
>> >> as the kernel should NOT be touching anything in a firmware blob other
>> >> than passing it off to the firmware directly.
>> >
>> > FWIW, the GSP firmware in question is already in linux-firmware and
>> > loaded by e.g. Nouveau.
>> >
>> > I am not sure how userspace could feed the proper ELF sections otherwise
>> > than by splitting the ELF binary into as many files as there are
>> > sections. Is that what you imply, or is there another means that would
>> > preserve the current firmware format?
>> >
>> > Note also that in this particular case, the kernel cannot just pass the
>> > firmware without modifying it anyway since the signatures relevant to
>> > the chipset need to be patched into the right place before it is loaded.
>> 
>> Quick nit, as that last statement was not entirely correct: while we do
>> patch some loaded firmware with signatures, this is not the case for the
>> GSP (the one in ELF format). Not that it changes the point, but for the
>> sake of accuracy. :)
>> 
>> The point being that even without using ELF as a container format, we do
>> need to parse header structures in loaded firmware files anyway, so the
>> kernel cannot simply act as a dumb pipe for firmware. And since we need
>> to add structure, let's at least use a format that is simple, well
>> accepted and which layout is already in the kernel.
>> 
>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
>
> The "problem" I'm not understanding is why does the kernel have to do
> any of this parsing at all?  What does it do with these segments that
> userspace can't do instead?  Why does patching have to be done within
> the kernel at all?  What prevents all of this from being done elsewhere?

I don't understand how userspace could do that unless we either 1) split
the firmwares into a multitude of files, or 2) rely on a daemon to do the
trivial processing that the kernel is currently doing.

A firmware like Booter (which loads the GSP) comes with a set of
signatures, one of which needs to be patched into the firmware data
segment. Which one to patch is inferred from a fuse register that is
read at probe time. So if we do 2), this means the kernel needs to
somehow communicate which signature to patch to the user-space daemon,
before it can get the patched firmware (which will still have some kind
of header with information like the entry point and values to write into
the boot ROM registers). This adds more dependencies just to bring up
the GPU and the processing that is done is so trivial that I don't see
the benefit of moving it to user-space, even though I do agree with the
general principle.

> And ELF parsing is "tricky" in places, and you aren't using the existing
> elf parser, as proof of needing a new parser in rust :)

We could also write an abstraction above one of the parsers that exist
on the C side, but I am not sure whether that would improve the safety
argument. :) This code is simple and easy to check (and pinky-swear
won't turn into something complex). Rust also helps with the safety
aspect - to some degree only, of course.

I have checked a few other drivers and many do at least some header
parsing from the firmware. I don't think what we are doing here is
particularly complex.

Re: [PATCH] rust: add basic ELF sections parser

[Timur Tabi]

On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
>
> The "problem" I'm not understanding is why does the kernel have to do
> any of this parsing at all?

Nova will need to parse ELF headers in order to properly load and boot
Nvidia firmware images.  Nouveau does this already:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931

In addition, we are adding new ELF image support to Nouveau to support
newer Hopper and Blackwell GPUs:

https://lore.kernel.org/nouveau/20250511210722.80350-55-bskeggs@nvidia.com/T/
(grep for "elf_header[]")

For this new code, I used ELF for new firmware images. I did this for
a few reasons:

1. In addition to the large GSP-RM binary, there are 3 "meta data"
blobs that are very small (<100 bytes) and I wanted a way to combine
all blobs into one file to keep things neat.
2. I didn't want to invent a new encapsulation format from scratch.
3. ELF allows you to use existing user-space tools to parse the file
if you are so inclined.
4. ELF contained all of the header bits I was looking for.
5. I was even able to include a CRC in the ELF header, for extra validation.
6. Structs for the headers already exist in the kernel and are used by
other drivers.



 What does it do with these segments that
> userspace can't do instead?  Why does patching have to be done within
> the kernel at all?  What prevents all of this from being done elsewhere?
>
> And ELF parsing is "tricky" in places, and you aren't using the existing
> elf parser, as proof of needing a new parser in rust :)
>
> thanks,
>
> greg k-h
>

Re: [PATCH] rust: add basic ELF sections parser

[John Hubbard]

On 5/15/25 7:30 AM, Timur Tabi wrote:
> On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
>>
>> The "problem" I'm not understanding is why does the kernel have to do
>> any of this parsing at all?
> 
> Nova will need to parse ELF headers in order to properly load and boot
> Nvidia firmware images.  Nouveau does this already:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931

Hi Greg!

Nouveau influenced us heavily here, because having firmware that we
can post once, and use everywhere (Nouveau and Nova), is very attractive.

Alex and Timur discuss other details that explain why the standard 
user-space approach is less simple and clean than it might appear at
first glance, but I wanted to emphasize that the firmware re-use point
a little bit, too.

Oh, and also: the ELF images are going to remain extremely simple,
because there is nothing now (nor can I see anything in the future)
that would drive anyone to do complicated things. For example, if
there is some exotic new thing in the future, it could be put into
its own firmware image if necessary--because we understand that
this parser here is intended to be a simple subset of ELF, and
left alone really.

thanks,
-- 
John Hubbard

> 
> In addition, we are adding new ELF image support to Nouveau to support
> newer Hopper and Blackwell GPUs:
> 
> https://lore.kernel.org/nouveau/20250511210722.80350-55-bskeggs@nvidia.com/T/
> (grep for "elf_header[]")
> 
> For this new code, I used ELF for new firmware images. I did this for
> a few reasons:
> 
> 1. In addition to the large GSP-RM binary, there are 3 "meta data"
> blobs that are very small (<100 bytes) and I wanted a way to combine
> all blobs into one file to keep things neat.
> 2. I didn't want to invent a new encapsulation format from scratch.
> 3. ELF allows you to use existing user-space tools to parse the file
> if you are so inclined.
> 4. ELF contained all of the header bits I was looking for.
> 5. I was even able to include a CRC in the ELF header, for extra validation.
> 6. Structs for the headers already exist in the kernel and are used by
> other drivers.
> 
> 
> 
>  What does it do with these segments that
>> userspace can't do instead?  Why does patching have to be done within
>> the kernel at all?  What prevents all of this from being done elsewhere?
>>
>> And ELF parsing is "tricky" in places, and you aren't using the existing
>> elf parser, as proof of needing a new parser in rust :)
>>
>> thanks,
>>
>> greg k-h
>>
> 

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Thu, May 15, 2025 at 12:17:00PM -0700, John Hubbard wrote:
> On 5/15/25 7:30 AM, Timur Tabi wrote:
> > On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
> >>
> >> The "problem" I'm not understanding is why does the kernel have to do
> >> any of this parsing at all?
> > 
> > Nova will need to parse ELF headers in order to properly load and boot
> > Nvidia firmware images.  Nouveau does this already:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931
> 
> Hi Greg!
> 
> Nouveau influenced us heavily here, because having firmware that we
> can post once, and use everywhere (Nouveau and Nova), is very attractive.
> 
> Alex and Timur discuss other details that explain why the standard 
> user-space approach is less simple and clean than it might appear at
> first glance, but I wanted to emphasize that the firmware re-use point
> a little bit, too.
> 
> Oh, and also: the ELF images are going to remain extremely simple,
> because there is nothing now (nor can I see anything in the future)
> that would drive anyone to do complicated things. For example, if
> there is some exotic new thing in the future, it could be put into
> its own firmware image if necessary--because we understand that
> this parser here is intended to be a simple subset of ELF, and
> left alone really.

Ok, then why not just bury this down in the driver that is going to
actually use it?  This patch series was adding it to ALL kernels, if you
need/want it or not, and as such would be seen as a generic way to
handle all ELF images.  But as that's not the case here, just copy what
you did in the existing C driver and make it private to your code, so
that no one else has to worry about accidentally thinking it would also
work for their code :)

And I still think that having the kernel do this is a mistake, firmware
should always just be a "pass through" otherwise you open yourself up to
all sorts of complexity and vulnerabilities in the kernel, both of which
is generally not a good idea.

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Fri May 16, 2025 at 10:15 PM JST, Greg KH wrote:
> On Thu, May 15, 2025 at 12:17:00PM -0700, John Hubbard wrote:
>> On 5/15/25 7:30 AM, Timur Tabi wrote:
>> > On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>> >>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
>> >>
>> >> The "problem" I'm not understanding is why does the kernel have to do
>> >> any of this parsing at all?
>> > 
>> > Nova will need to parse ELF headers in order to properly load and boot
>> > Nvidia firmware images.  Nouveau does this already:
>> > 
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931
>> 
>> Hi Greg!
>> 
>> Nouveau influenced us heavily here, because having firmware that we
>> can post once, and use everywhere (Nouveau and Nova), is very attractive.
>> 
>> Alex and Timur discuss other details that explain why the standard 
>> user-space approach is less simple and clean than it might appear at
>> first glance, but I wanted to emphasize that the firmware re-use point
>> a little bit, too.
>> 
>> Oh, and also: the ELF images are going to remain extremely simple,
>> because there is nothing now (nor can I see anything in the future)
>> that would drive anyone to do complicated things. For example, if
>> there is some exotic new thing in the future, it could be put into
>> its own firmware image if necessary--because we understand that
>> this parser here is intended to be a simple subset of ELF, and
>> left alone really.
>
> Ok, then why not just bury this down in the driver that is going to
> actually use it?  This patch series was adding it to ALL kernels, if you
> need/want it or not, and as such would be seen as a generic way to
> handle all ELF images.  But as that's not the case here, just copy what
> you did in the existing C driver and make it private to your code, so
> that no one else has to worry about accidentally thinking it would also
> work for their code :)

Keeping this local to nova-core is perfectly fine if you think this is
more acceptable. AFAIK there are no other users for it at the moment.

> And I still think that having the kernel do this is a mistake, firmware
> should always just be a "pass through" otherwise you open yourself up to
> all sorts of complexity and vulnerabilities in the kernel, both of which
> is generally not a good idea.

I agree on principle, but I cannot think of a way to avoid doing this in
the kernel without making things overly complex. We're happy to consider
alternatives though, if they exist.

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 16, 2025 at 10:26:10PM +0900, Alexandre Courbot wrote:
> On Fri May 16, 2025 at 10:15 PM JST, Greg KH wrote:
> > On Thu, May 15, 2025 at 12:17:00PM -0700, John Hubbard wrote:
> >> On 5/15/25 7:30 AM, Timur Tabi wrote:
> >> > On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
> >> >>
> >> >> The "problem" I'm not understanding is why does the kernel have to do
> >> >> any of this parsing at all?
> >> > 
> >> > Nova will need to parse ELF headers in order to properly load and boot
> >> > Nvidia firmware images.  Nouveau does this already:
> >> > 
> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931
> >> 
> >> Hi Greg!
> >> 
> >> Nouveau influenced us heavily here, because having firmware that we
> >> can post once, and use everywhere (Nouveau and Nova), is very attractive.
> >> 
> >> Alex and Timur discuss other details that explain why the standard 
> >> user-space approach is less simple and clean than it might appear at
> >> first glance, but I wanted to emphasize that the firmware re-use point
> >> a little bit, too.
> >> 
> >> Oh, and also: the ELF images are going to remain extremely simple,
> >> because there is nothing now (nor can I see anything in the future)
> >> that would drive anyone to do complicated things. For example, if
> >> there is some exotic new thing in the future, it could be put into
> >> its own firmware image if necessary--because we understand that
> >> this parser here is intended to be a simple subset of ELF, and
> >> left alone really.
> >
> > Ok, then why not just bury this down in the driver that is going to
> > actually use it?  This patch series was adding it to ALL kernels, if you
> > need/want it or not, and as such would be seen as a generic way to
> > handle all ELF images.  But as that's not the case here, just copy what
> > you did in the existing C driver and make it private to your code, so
> > that no one else has to worry about accidentally thinking it would also
> > work for their code :)
> 
> Keeping this local to nova-core is perfectly fine if you think this is
> more acceptable. AFAIK there are no other users for it at the moment.

Then I recommend doing this, thanks.

> > And I still think that having the kernel do this is a mistake, firmware
> > should always just be a "pass through" otherwise you open yourself up to
> > all sorts of complexity and vulnerabilities in the kernel, both of which
> > is generally not a good idea.
> 
> I agree on principle, but I cannot think of a way to avoid doing this in
> the kernel without making things overly complex. We're happy to consider
> alternatives though, if they exist.

With the exception of the "set the fuse issue", I really don't see why
sending a bunch of small firmware files through the kernel to the
hardware is any harder to do in userspace vs. within the kernel itself.
Either way you have to do the "parsing" and splitting up, it's just
generally safer to do all of that in userspace instead.

But you all have a beast of a device to control, odds are this is one of
the only tiny minor issues involved with that overall :)

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Danilo Krummrich]

On Fri, May 16, 2025 at 10:26:10PM +0900, Alexandre Courbot wrote:
> On Fri May 16, 2025 at 10:15 PM JST, Greg KH wrote:
> > On Thu, May 15, 2025 at 12:17:00PM -0700, John Hubbard wrote:
> >> On 5/15/25 7:30 AM, Timur Tabi wrote:
> >> > On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
> >> >>
> >> >> The "problem" I'm not understanding is why does the kernel have to do
> >> >> any of this parsing at all?
> >> > 
> >> > Nova will need to parse ELF headers in order to properly load and boot
> >> > Nvidia firmware images.  Nouveau does this already:
> >> > 
> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931
> >> 
> >> Hi Greg!
> >> 
> >> Nouveau influenced us heavily here, because having firmware that we
> >> can post once, and use everywhere (Nouveau and Nova), is very attractive.
> >> 
> >> Alex and Timur discuss other details that explain why the standard 
> >> user-space approach is less simple and clean than it might appear at
> >> first glance, but I wanted to emphasize that the firmware re-use point
> >> a little bit, too.
> >> 
> >> Oh, and also: the ELF images are going to remain extremely simple,
> >> because there is nothing now (nor can I see anything in the future)
> >> that would drive anyone to do complicated things. For example, if
> >> there is some exotic new thing in the future, it could be put into
> >> its own firmware image if necessary--because we understand that
> >> this parser here is intended to be a simple subset of ELF, and
> >> left alone really.
> >
> > Ok, then why not just bury this down in the driver that is going to
> > actually use it?  This patch series was adding it to ALL kernels, if you
> > need/want it or not, and as such would be seen as a generic way to
> > handle all ELF images.  But as that's not the case here, just copy what
> > you did in the existing C driver and make it private to your code, so
> > that no one else has to worry about accidentally thinking it would also
> > work for their code :)
> 
> Keeping this local to nova-core is perfectly fine if you think this is
> more acceptable. AFAIK there are no other users for it at the moment.

I'm not quite on board with that.

I think we should either we get to the conclusion that the desire of parsing (at
least part of) the firmware ELF is valid in the kernel and make it generic
infrastructure, or conclude that there really isn't a reasonable technical
reason to do that.

Please let's work out the exact technical reasons for doing this in the kernel,
such that we can either conclude one or the other.

> > And I still think that having the kernel do this is a mistake, firmware
> > should always just be a "pass through" otherwise you open yourself up to
> > all sorts of complexity and vulnerabilities in the kernel, both of which
> > is generally not a good idea.
> 
> I agree on principle, but I cannot think of a way to avoid doing this in
> the kernel without making things overly complex. We're happy to consider
> alternatives though, if they exist.

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Fri May 16, 2025 at 10:35 PM JST, Danilo Krummrich wrote:
> On Fri, May 16, 2025 at 10:26:10PM +0900, Alexandre Courbot wrote:
>> On Fri May 16, 2025 at 10:15 PM JST, Greg KH wrote:
>> > On Thu, May 15, 2025 at 12:17:00PM -0700, John Hubbard wrote:
>> >> On 5/15/25 7:30 AM, Timur Tabi wrote:
>> >> > On Thu, May 15, 2025 at 6:43 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> >>> Or if ELF is the problem, I don't mind introducing a WAD loader. ;)
>> >> >>
>> >> >> The "problem" I'm not understanding is why does the kernel have to do
>> >> >> any of this parsing at all?
>> >> > 
>> >> > Nova will need to parse ELF headers in order to properly load and boot
>> >> > Nvidia firmware images.  Nouveau does this already:
>> >> > 
>> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c#n2931
>> >> 
>> >> Hi Greg!
>> >> 
>> >> Nouveau influenced us heavily here, because having firmware that we
>> >> can post once, and use everywhere (Nouveau and Nova), is very attractive.
>> >> 
>> >> Alex and Timur discuss other details that explain why the standard 
>> >> user-space approach is less simple and clean than it might appear at
>> >> first glance, but I wanted to emphasize that the firmware re-use point
>> >> a little bit, too.
>> >> 
>> >> Oh, and also: the ELF images are going to remain extremely simple,
>> >> because there is nothing now (nor can I see anything in the future)
>> >> that would drive anyone to do complicated things. For example, if
>> >> there is some exotic new thing in the future, it could be put into
>> >> its own firmware image if necessary--because we understand that
>> >> this parser here is intended to be a simple subset of ELF, and
>> >> left alone really.
>> >
>> > Ok, then why not just bury this down in the driver that is going to
>> > actually use it?  This patch series was adding it to ALL kernels, if you
>> > need/want it or not, and as such would be seen as a generic way to
>> > handle all ELF images.  But as that's not the case here, just copy what
>> > you did in the existing C driver and make it private to your code, so
>> > that no one else has to worry about accidentally thinking it would also
>> > work for their code :)
>> 
>> Keeping this local to nova-core is perfectly fine if you think this is
>> more acceptable. AFAIK there are no other users for it at the moment.
>
> I'm not quite on board with that.
>
> I think we should either we get to the conclusion that the desire of parsing (at
> least part of) the firmware ELF is valid in the kernel and make it generic
> infrastructure, or conclude that there really isn't a reasonable technical
> reason to do that.
>
> Please let's work out the exact technical reasons for doing this in the kernel,
> such that we can either conclude one or the other.

I think it's mostly a matter of where we want to draw the line.

We use ELF as a container format to associate binary blobs with named
sections. Can we extract these sections into individual files that we
load using request_firmware()? Why yes, we could.

Now the GSP firmware for GA102 contains the following sections (skipped
the ones that don't need to be extracted):

  [ 1] .fwimage          PROGBITS         0000000000000000  00000040
  [ 2] .fwversion        PROGBITS         0000000000000000  02448040
  [ 3] .fwsignature[...] PROGBITS         0000000000000000  0244804b
  [ 4] .fwsignature[...] PROGBITS         0000000000000000  0244904b
  [ 5] .fwsignature[...] PROGBITS         0000000000000000  0244a04b
  [ 6] .fwsignature[...] PROGBITS         0000000000000000  0244b04b

That's 6 files instead of 1, for serving the same purpose. And the number of
signatures is bound to *increase* as new chips get released, but since they are
associated to chipsets, we can maybe limit them to the relevant chipset
directory and limit the damage. Still it would clutter linux-firmware a bit
more than it is today.

But let's say we do this, and problem solved. Only... let's take a look at the
booter binary, which is another innocent-looking firmware file.

It includes a header with offsets to the code and data segments, that the
driver loads into the falcon microcontroller. And one offset for the signatures
that we need to patch. Reminds you of something? :) Should we split these ones
too?

I would push back really hard on that one, unless you agree to go after all the
drivers that do the same thing (and I have names). But how is it different from
what we are doing with ELF? We are just indexing a byte stream using indices in
headers, while carefully checking the bounds.

At the end of the day, it's a tradeoff. But I feel like our only sin is using a
standard file format that gets visibility instead of rolling our own header
that would go under the radar.

Re: [PATCH] rust: add basic ELF sections parser

[Danilo Krummrich]

On Fri, May 16, 2025 at 11:35:42PM +0900, Alexandre Courbot wrote:
> On Fri May 16, 2025 at 10:35 PM JST, Danilo Krummrich wrote:
> > I think we should either we get to the conclusion that the desire of parsing (at
> > least part of) the firmware ELF is valid in the kernel and make it generic
> > infrastructure, or conclude that there really isn't a reasonable technical
> > reason to do that.
> >
> > Please let's work out the exact technical reasons for doing this in the kernel,
> > such that we can either conclude one or the other.
> 
> I think it's mostly a matter of where we want to draw the line.
> 
> We use ELF as a container format to associate binary blobs with named
> sections. Can we extract these sections into individual files that we
> load using request_firmware()? Why yes, we could.
> 
> Now the GSP firmware for GA102 contains the following sections (skipped
> the ones that don't need to be extracted):
> 
>   [ 1] .fwimage          PROGBITS         0000000000000000  00000040
>   [ 2] .fwversion        PROGBITS         0000000000000000  02448040
>   [ 3] .fwsignature[...] PROGBITS         0000000000000000  0244804b
>   [ 4] .fwsignature[...] PROGBITS         0000000000000000  0244904b
>   [ 5] .fwsignature[...] PROGBITS         0000000000000000  0244a04b
>   [ 6] .fwsignature[...] PROGBITS         0000000000000000  0244b04b
> 
> That's 6 files instead of 1, for serving the same purpose. And the number of
> signatures is bound to *increase* as new chips get released, but since they are
> associated to chipsets, we can maybe limit them to the relevant chipset
> directory and limit the damage. Still it would clutter linux-firmware a bit
> more than it is today.
> 
> But let's say we do this, and problem solved. Only... let's take a look at the
> booter binary, which is another innocent-looking firmware file.
> 
> It includes a header with offsets to the code and data segments, that the
> driver loads into the falcon microcontroller. And one offset for the signatures
> that we need to patch. Reminds you of something? :) Should we split these ones
> too?
> 
> I would push back really hard on that one, unless you agree to go after all the
> drivers that do the same thing (and I have names).

Great, but then why did you back off in your discussion with Greg? Given that
you are convinced that this is a valid thing to do for drivers, you should keep
discussing it with the target to make it common infrastructure.

I did not argue for or against it -- what I do disagree with is that we seem to
just agree to disagree and stuff a generic piece of code into nova-core after
three mails back and forth.

Please keep discussing the above with Greg until we get to a real conclusion.

- Danilo

Re: [PATCH] rust: add basic ELF sections parser

[Timur Tabi]

On Fri, May 16, 2025 at 9:35 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> We use ELF as a container format to associate binary blobs with named
> sections. Can we extract these sections into individual files that we
> load using request_firmware()? Why yes, we could.

Actually, I don't think we can.  This is the actual GSP-RM ELF image
you're talking about.  This comes packaged as one binary blob and it's
intended to be mostly opaque.  We can't just disassemble the ELF
sections and then re-assemble them in the driver.

Unfortunately, for pre-Hopper booting, we need to do a little
pre-processing on the image, referencing the ELF sections, and based
on data from fuses that cannot be read in user-space.

Re: [PATCH] rust: add basic ELF sections parser

[John Hubbard]

On 5/16/25 9:01 AM, Danilo Krummrich wrote:
> On Fri, May 16, 2025 at 11:35:42PM +0900, Alexandre Courbot wrote:
>> On Fri May 16, 2025 at 10:35 PM JST, Danilo Krummrich wrote:
...
>> Now the GSP firmware for GA102 contains the following sections (skipped
>> the ones that don't need to be extracted):
>>
>>    [ 1] .fwimage          PROGBITS         0000000000000000  00000040
>>    [ 2] .fwversion        PROGBITS         0000000000000000  02448040
>>    [ 3] .fwsignature[...] PROGBITS         0000000000000000  0244804b
>>    [ 4] .fwsignature[...] PROGBITS         0000000000000000  0244904b
>>    [ 5] .fwsignature[...] PROGBITS         0000000000000000  0244a04b
>>    [ 6] .fwsignature[...] PROGBITS         0000000000000000  0244b04b
>>
>> That's 6 files instead of 1, for serving the same purpose. And the number of

gaahhh!

We are already at 252 files, *before* artificially exploding the GSP
into sub-files:

$ find /lib/firmware/nvidia -type f | wc -l
252

...
>> I would push back really hard on that one, unless you agree to go after all the
>> drivers that do the same thing (and I have names).
> 
> Great, but then why did you back off in your discussion with Greg? Given that
> you are convinced that this is a valid thing to do for drivers, you should keep
> discussing it with the target to make it common infrastructure.
> 

One way forward would be to apply this to one or two other drivers.
Alex, you said you have names, can you please list those names here?

> I did not argue for or against it -- what I do disagree with is that we seem to
> just agree to disagree and stuff a generic piece of code into nova-core after
> three mails back and forth.
> 
> Please keep discussing the above with Greg until we get to a real conclusion.
> 
> - Danilo

Danilo, this is a small amount of code. Is there any real problem with
a compromise, that starts out with Greg's approach of putting this code
in Nova?

And then we can promote it if and when we can make a good case for that.

But in the meantime, we really need to be able to take "yes" (from Greg)
for an answer! I don't want to blow up the firmware images, shred
the "Nouveau and Nova can use the same images" story, and delay merging,
all over 100 lines of code.

Yes?

thanks,
-- 
John Hubbard

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Sat May 17, 2025 at 1:28 AM JST, Timur Tabi wrote:
> On Fri, May 16, 2025 at 9:35 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> We use ELF as a container format to associate binary blobs with named
>> sections. Can we extract these sections into individual files that we
>> load using request_firmware()? Why yes, we could.
>
> Actually, I don't think we can.  This is the actual GSP-RM ELF image
> you're talking about.  This comes packaged as one binary blob and it's
> intended to be mostly opaque.  We can't just disassemble the ELF
> sections and then re-assemble them in the driver.
>
> Unfortunately, for pre-Hopper booting, we need to do a little
> pre-processing on the image, referencing the ELF sections, and based
> on data from fuses that cannot be read in user-space.

I'd like to reinforce Timur's point a bit because it is crucial to
understanding why we need an ELF parser here.

On post-Hopper, the GSP ELF binary is passed as-is to the booter
firmware and it is the latter that performs the blob extraction from the
ELF sections. So for these chips no ELF parsing takes place in the
kernel which actually acts as a dumb pipe.

However, pre-Hopper does not work like that, and for these the same GSP
image (coming from the same ELF file) needs to be extracted by the
kernel and handed out to booter. It's for these that we need to do the
light parsing introduced by this patch.

So while I believe this provides a strong justification for having the
parser, I also understand Greg's reluctance to make this available to
everyone when nova-core is the only user in sight and the general
guideline is to avoid processing in the kernel.

OTOH, it is quite short and trivial, and if some drivers need a
packaging format then it might as well be ELF. The imagination DRM
driver for instance appears to load firmware parts from an ELF binary
obtained using request_firmware (lookup `process_elf_command_stream`) -
very similar to what we are doing here.

`drivers/remoteproc` also has what appears to be a complete ELF parser
and loader, which it uses on firmware obtained using `request_firmware`
(check `remoteproc_elf_loader.c` and how the arguments to the functions
defined there are `struct firmware *`). Admittedly, it's probably easier
to justify here, but the core principle is the same and we are just
doing a much simpler version of that.

And there are likely more examples, so there might be a case for a
shared ELF parser. For nova-core purposes, either way would work.

Re: [PATCH] rust: add basic ELF sections parser

[Danilo Krummrich]

On Fri, May 16, 2025 at 12:00:11PM -0700, John Hubbard wrote:
> On 5/16/25 9:01 AM, Danilo Krummrich wrote:
> > I did not argue for or against it -- what I do disagree with is that we seem to
> > just agree to disagree and stuff a generic piece of code into nova-core after
> > three mails back and forth.
> > 
> > Please keep discussing the above with Greg until we get to a real conclusion.
> 
> Danilo, this is a small amount of code. Is there any real problem with
> a compromise, that starts out with Greg's approach of putting this code
> in Nova?
> 
> And then we can promote it if and when we can make a good case for that.
> 
> But in the meantime, we really need to be able to take "yes" (from Greg)
> for an answer! I don't want to blow up the firmware images, shred
> the "Nouveau and Nova can use the same images" story, and delay merging,
> all over 100 lines of code.

What I disagree with is that it seems that you actually *can* "make a good case
for it", but stop discussing it with Greg, because he would agree with just
stuffing this generic infrastructure in the driver.

It seems like taking the way of least resistance even though you seem to have
good arguments for this to be generic infrastructure.

The fact that *only* nova-core would use it to begin with is not the relevant
factor. nova-core is, for obvious reasons, the only user of quite some generic
infrastructure, yet we don't stuff it into the driver.

The relevant factor is, do we agree that this is a valid requirement for drivers
in general (which you seem to answer with a clear "yes"). Which means we should
keep discussing it.

If we can't conclude that this should be generic infrastructure, we can still
figure out the way forward for nova-core (i.e. have our own ELF parser in
nova-core if necessary).

But, as I said above, please don't stop the discussion with agreeing to disagree
after three mails back and forth.

We have plenty of time to discuss it; we do not target v6.16 and the v6.17 merge
window is far away.

- Danilo

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Sat May 17, 2025 at 7:13 PM JST, Danilo Krummrich wrote:
> On Fri, May 16, 2025 at 12:00:11PM -0700, John Hubbard wrote:
>> On 5/16/25 9:01 AM, Danilo Krummrich wrote:
>> > I did not argue for or against it -- what I do disagree with is that we seem to
>> > just agree to disagree and stuff a generic piece of code into nova-core after
>> > three mails back and forth.
>> > 
>> > Please keep discussing the above with Greg until we get to a real conclusion.
>> 
>> Danilo, this is a small amount of code. Is there any real problem with
>> a compromise, that starts out with Greg's approach of putting this code
>> in Nova?
>> 
>> And then we can promote it if and when we can make a good case for that.
>> 
>> But in the meantime, we really need to be able to take "yes" (from Greg)
>> for an answer! I don't want to blow up the firmware images, shred
>> the "Nouveau and Nova can use the same images" story, and delay merging,
>> all over 100 lines of code.
>
> What I disagree with is that it seems that you actually *can* "make a good case
> for it", but stop discussing it with Greg, because he would agree with just
> stuffing this generic infrastructure in the driver.
>
> It seems like taking the way of least resistance even though you seem to have
> good arguments for this to be generic infrastructure.
>
> The fact that *only* nova-core would use it to begin with is not the relevant
> factor. nova-core is, for obvious reasons, the only user of quite some generic
> infrastructure, yet we don't stuff it into the driver.
>
> The relevant factor is, do we agree that this is a valid requirement for drivers
> in general (which you seem to answer with a clear "yes"). Which means we should
> keep discussing it.
>
> If we can't conclude that this should be generic infrastructure, we can still
> figure out the way forward for nova-core (i.e. have our own ELF parser in
> nova-core if necessary).
>
> But, as I said above, please don't stop the discussion with agreeing to disagree
> after three mails back and forth.
>
> We have plenty of time to discuss it; we do not target v6.16 and the v6.17 merge
> window is far away.

I've tried to make the case for this being generic infrastructure in my
other email, the TL;DR being that there is precedent with at least three
C drivers (if we include Nouveau), likely a few more - and I haven't yet
looked for drivers doing the same thing with their own container format,
there might be a few surprises here as well. ^_^;

You are right that we are not in a hurry, and having this discussion
should not slow Nova's progress (which is our main concern). But should
the decision take some time, let's keep in mind that moving the module
out of nova-core into the kernel crate requires little more than a
matter of doing `git mv`, so there is little friction in having it
locally first.

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

Hi Greg,

On Sat May 17, 2025 at 9:51 AM JST, Alexandre Courbot wrote:
> On Sat May 17, 2025 at 1:28 AM JST, Timur Tabi wrote:
>> On Fri, May 16, 2025 at 9:35 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>>
>>> We use ELF as a container format to associate binary blobs with named
>>> sections. Can we extract these sections into individual files that we
>>> load using request_firmware()? Why yes, we could.
>>
>> Actually, I don't think we can.  This is the actual GSP-RM ELF image
>> you're talking about.  This comes packaged as one binary blob and it's
>> intended to be mostly opaque.  We can't just disassemble the ELF
>> sections and then re-assemble them in the driver.
>>
>> Unfortunately, for pre-Hopper booting, we need to do a little
>> pre-processing on the image, referencing the ELF sections, and based
>> on data from fuses that cannot be read in user-space.
>
> I'd like to reinforce Timur's point a bit because it is crucial to
> understanding why we need an ELF parser here.
>
> On post-Hopper, the GSP ELF binary is passed as-is to the booter
> firmware and it is the latter that performs the blob extraction from the
> ELF sections. So for these chips no ELF parsing takes place in the
> kernel which actually acts as a dumb pipe.
>
> However, pre-Hopper does not work like that, and for these the same GSP
> image (coming from the same ELF file) needs to be extracted by the
> kernel and handed out to booter. It's for these that we need to do the
> light parsing introduced by this patch.
>
> So while I believe this provides a strong justification for having the
> parser, I also understand Greg's reluctance to make this available to
> everyone when nova-core is the only user in sight and the general
> guideline is to avoid processing in the kernel.
>
> OTOH, it is quite short and trivial, and if some drivers need a
> packaging format then it might as well be ELF. The imagination DRM
> driver for instance appears to load firmware parts from an ELF binary
> obtained using request_firmware (lookup `process_elf_command_stream`) -
> very similar to what we are doing here.
>
> `drivers/remoteproc` also has what appears to be a complete ELF parser
> and loader, which it uses on firmware obtained using `request_firmware`
> (check `remoteproc_elf_loader.c` and how the arguments to the functions
> defined there are `struct firmware *`). Admittedly, it's probably easier
> to justify here, but the core principle is the same and we are just
> doing a much simpler version of that.
>
> And there are likely more examples, so there might be a case for a
> shared ELF parser. For nova-core purposes, either way would work.

Gentle ping on this, as you can there are other drivers using ELF as a
container format for firmware. In light of this information, I guess
there is a point for having a common parser in the kernel. What do you
think?

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Thu, May 29, 2025 at 03:53:42PM +0900, Alexandre Courbot wrote:
> Hi Greg,
> 
> On Sat May 17, 2025 at 9:51 AM JST, Alexandre Courbot wrote:
> > On Sat May 17, 2025 at 1:28 AM JST, Timur Tabi wrote:
> >> On Fri, May 16, 2025 at 9:35 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
> >>>
> >>> We use ELF as a container format to associate binary blobs with named
> >>> sections. Can we extract these sections into individual files that we
> >>> load using request_firmware()? Why yes, we could.
> >>
> >> Actually, I don't think we can.  This is the actual GSP-RM ELF image
> >> you're talking about.  This comes packaged as one binary blob and it's
> >> intended to be mostly opaque.  We can't just disassemble the ELF
> >> sections and then re-assemble them in the driver.
> >>
> >> Unfortunately, for pre-Hopper booting, we need to do a little
> >> pre-processing on the image, referencing the ELF sections, and based
> >> on data from fuses that cannot be read in user-space.
> >
> > I'd like to reinforce Timur's point a bit because it is crucial to
> > understanding why we need an ELF parser here.
> >
> > On post-Hopper, the GSP ELF binary is passed as-is to the booter
> > firmware and it is the latter that performs the blob extraction from the
> > ELF sections. So for these chips no ELF parsing takes place in the
> > kernel which actually acts as a dumb pipe.
> >
> > However, pre-Hopper does not work like that, and for these the same GSP
> > image (coming from the same ELF file) needs to be extracted by the
> > kernel and handed out to booter. It's for these that we need to do the
> > light parsing introduced by this patch.
> >
> > So while I believe this provides a strong justification for having the
> > parser, I also understand Greg's reluctance to make this available to
> > everyone when nova-core is the only user in sight and the general
> > guideline is to avoid processing in the kernel.
> >
> > OTOH, it is quite short and trivial, and if some drivers need a
> > packaging format then it might as well be ELF. The imagination DRM
> > driver for instance appears to load firmware parts from an ELF binary
> > obtained using request_firmware (lookup `process_elf_command_stream`) -
> > very similar to what we are doing here.
> >
> > `drivers/remoteproc` also has what appears to be a complete ELF parser
> > and loader, which it uses on firmware obtained using `request_firmware`
> > (check `remoteproc_elf_loader.c` and how the arguments to the functions
> > defined there are `struct firmware *`). Admittedly, it's probably easier
> > to justify here, but the core principle is the same and we are just
> > doing a much simpler version of that.
> >
> > And there are likely more examples, so there might be a case for a
> > shared ELF parser. For nova-core purposes, either way would work.
> 
> Gentle ping on this, as you can there are other drivers using ELF as a
> container format for firmware. In light of this information, I guess
> there is a point for having a common parser in the kernel. What do you
> think?
> 

I think that the other examples should be fixed up to not do that :)

remoteproc is one example, that elf logic should all be done in
userspace, but as it's been in the tree "for forever", changing it is
not going to be possible.

Same for the existing users, changing their user/kernel api is not going
to be a simple task given that there are running systems relying on
them.

But, going forward, I think you need an explicit "this is the ONLY way
we can do this so it MUST be in the kernel" justification for adding
this type of api.  AND if that happens, THEN it should be in generic
code ONCE there are multiple users of it.

But for now, doing it in generic code, that all systems end up loading,
yet very very very few would ever actually use makes no sense.  And
adding it to a driver also doesn't make sense as you can define your
user/kernel api now, it's not set in stone at all given that there is no
existing code merged.

So again, I'm all for "do NOT do yet-another-elf-loader-in-the-kernel",
if asked.

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Thu May 29, 2025 at 5:01 PM JST, Greg KH wrote:
> On Thu, May 29, 2025 at 03:53:42PM +0900, Alexandre Courbot wrote:
>> Hi Greg,
>> 
>> On Sat May 17, 2025 at 9:51 AM JST, Alexandre Courbot wrote:
>> > On Sat May 17, 2025 at 1:28 AM JST, Timur Tabi wrote:
>> >> On Fri, May 16, 2025 at 9:35 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>> >>>
>> >>> We use ELF as a container format to associate binary blobs with named
>> >>> sections. Can we extract these sections into individual files that we
>> >>> load using request_firmware()? Why yes, we could.
>> >>
>> >> Actually, I don't think we can.  This is the actual GSP-RM ELF image
>> >> you're talking about.  This comes packaged as one binary blob and it's
>> >> intended to be mostly opaque.  We can't just disassemble the ELF
>> >> sections and then re-assemble them in the driver.
>> >>
>> >> Unfortunately, for pre-Hopper booting, we need to do a little
>> >> pre-processing on the image, referencing the ELF sections, and based
>> >> on data from fuses that cannot be read in user-space.
>> >
>> > I'd like to reinforce Timur's point a bit because it is crucial to
q> > understanding why we need an ELF parser here.
>> >
>> > On post-Hopper, the GSP ELF binary is passed as-is to the booter
>> > firmware and it is the latter that performs the blob extraction from the
>> > ELF sections. So for these chips no ELF parsing takes place in the
>> > kernel which actually acts as a dumb pipe.
>> >
>> > However, pre-Hopper does not work like that, and for these the same GSP
>> > image (coming from the same ELF file) needs to be extracted by the
>> > kernel and handed out to booter. It's for these that we need to do the
>> > light parsing introduced by this patch.
>> >
>> > So while I believe this provides a strong justification for having the
>> > parser, I also understand Greg's reluctance to make this available to
>> > everyone when nova-core is the only user in sight and the general
>> > guideline is to avoid processing in the kernel.
>> >
>> > OTOH, it is quite short and trivial, and if some drivers need a
>> > packaging format then it might as well be ELF. The imagination DRM
>> > driver for instance appears to load firmware parts from an ELF binary
>> > obtained using request_firmware (lookup `process_elf_command_stream`) -
>> > very similar to what we are doing here.
>> >
>> > `drivers/remoteproc` also has what appears to be a complete ELF parser
>> > and loader, which it uses on firmware obtained using `request_firmware`
>> > (check `remoteproc_elf_loader.c` and how the arguments to the functions
>> > defined there are `struct firmware *`). Admittedly, it's probably easier
>> > to justify here, but the core principle is the same and we are just
>> > doing a much simpler version of that.
>> >
>> > And there are likely more examples, so there might be a case for a
>> > shared ELF parser. For nova-core purposes, either way would work.
>> 
>> Gentle ping on this, as you can there are other drivers using ELF as a
>> container format for firmware. In light of this information, I guess
>> there is a point for having a common parser in the kernel. What do you
>> think?
>> 
>
> I think that the other examples should be fixed up to not do that :)
>
> remoteproc is one example, that elf logic should all be done in
> userspace, but as it's been in the tree "for forever", changing it is
> not going to be possible.
>
> Same for the existing users, changing their user/kernel api is not going
> to be a simple task given that there are running systems relying on
> them.
>
> But, going forward, I think you need an explicit "this is the ONLY way
> we can do this so it MUST be in the kernel" justification for adding
> this type of api.

I think we do have such a case with Nova. On Hopper+ chips, the loaded
ELF binary is passed as-is to the GSP, which does the unpacking itself -
so no parsing needs to be done by the kernel whatsoever.

However, Nova also supports a couple of older chip generations that use
the same GSP firmware -  it is for these that the ELF unpacking must
occur in the kernel. IIUC this has to do with the capabilities of the
microcontroller that ultimately does the loading (more capable RISC-V on
Hopper+ vs. older and more limited Falcon).

So the "good news" is that this parser is only needed for 2 families of
older chips, with newer (and future) ones not exercising it at all.

> AND if that happens, THEN it should be in generic
> code ONCE there are multiple users of it.

Ok, we can definitely limit this to Nova.

>
> But for now, doing it in generic code, that all systems end up loading,
> yet very very very few would ever actually use makes no sense.  And
> adding it to a driver also doesn't make sense as you can define your
> user/kernel api now, it's not set in stone at all given that there is no
> existing code merged.

Eschewing this from the driver would require duplicating the GSP
firmware (a healthy 26MB compressed binary) in linux-firmware to provide
both ELF and non-ELF versions of the same code, and also store the other
ELF sections as their own files. I expect this to be a hard sell for
linux-firmware.

Cheers,
Alex.

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
> > But for now, doing it in generic code, that all systems end up loading,
> > yet very very very few would ever actually use makes no sense.  And
> > adding it to a driver also doesn't make sense as you can define your
> > user/kernel api now, it's not set in stone at all given that there is no
> > existing code merged.
> 
> Eschewing this from the driver would require duplicating the GSP
> firmware (a healthy 26MB compressed binary) in linux-firmware to provide
> both ELF and non-ELF versions of the same code, and also store the other
> ELF sections as their own files. I expect this to be a hard sell for
> linux-firmware.

Why would the linux-firmware people care about the size of firmware
blobs being given to them?  That's the whole reason for their existance,
to put them in one place instead of having to download them from random
locations on the internet, or to have them in the kernel tree itself.

It's already 300MB or so for the whole project, what's 26MB more?  It's
not like the collection is ever going to get smaller :)

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
> However, Nova also supports a couple of older chip generations that use
> the same GSP firmware -  it is for these that the ELF unpacking must
> occur in the kernel. IIUC this has to do with the capabilities of the
> microcontroller that ultimately does the loading (more capable RISC-V on
> Hopper+ vs. older and more limited Falcon).

Why specifically does the kernel have to get involved here?  What
requires it to do it that userspace can not?

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Fri May 30, 2025 at 3:21 PM JST, Greg KH wrote:
> On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
>> > But for now, doing it in generic code, that all systems end up loading,
>> > yet very very very few would ever actually use makes no sense.  And
>> > adding it to a driver also doesn't make sense as you can define your
>> > user/kernel api now, it's not set in stone at all given that there is no
>> > existing code merged.
>> 
>> Eschewing this from the driver would require duplicating the GSP
>> firmware (a healthy 26MB compressed binary) in linux-firmware to provide
>> both ELF and non-ELF versions of the same code, and also store the other
>> ELF sections as their own files. I expect this to be a hard sell for
>> linux-firmware.
>
> Why would the linux-firmware people care about the size of firmware
> blobs being given to them?  That's the whole reason for their existance,
> to put them in one place instead of having to download them from random
> locations on the internet, or to have them in the kernel tree itself.
>
> It's already 300MB or so for the whole project, what's 26MB more?

Roughtly 1/10th of the current total size as avoidable overhead. ^_^;

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Fri May 30, 2025 at 3:22 PM JST, Greg KH wrote:
> On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
>> However, Nova also supports a couple of older chip generations that use
>> the same GSP firmware -  it is for these that the ELF unpacking must
>> occur in the kernel. IIUC this has to do with the capabilities of the
>> microcontroller that ultimately does the loading (more capable RISC-V on
>> Hopper+ vs. older and more limited Falcon).
>
> Why specifically does the kernel have to get involved here?  What
> requires it to do it that userspace can not?

I don't know of a user-space tool that is readily available and could
perform such extraction of the ELF content upon kernel request. Is there
anything like this?

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 30, 2025 at 03:56:37PM +0900, Alexandre Courbot wrote:
> On Fri May 30, 2025 at 3:21 PM JST, Greg KH wrote:
> > On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
> >> > But for now, doing it in generic code, that all systems end up loading,
> >> > yet very very very few would ever actually use makes no sense.  And
> >> > adding it to a driver also doesn't make sense as you can define your
> >> > user/kernel api now, it's not set in stone at all given that there is no
> >> > existing code merged.
> >> 
> >> Eschewing this from the driver would require duplicating the GSP
> >> firmware (a healthy 26MB compressed binary) in linux-firmware to provide
> >> both ELF and non-ELF versions of the same code, and also store the other
> >> ELF sections as their own files. I expect this to be a hard sell for
> >> linux-firmware.
> >
> > Why would the linux-firmware people care about the size of firmware
> > blobs being given to them?  That's the whole reason for their existance,
> > to put them in one place instead of having to download them from random
> > locations on the internet, or to have them in the kernel tree itself.
> >
> > It's already 300MB or so for the whole project, what's 26MB more?
> 
> Roughtly 1/10th of the current total size as avoidable overhead. ^_^;

It's just storage on a disk, that's not an issue.  Storage sizes just
increased by more than that in the days we've taken on this email
thread :)

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 30, 2025 at 03:59:03PM +0900, Alexandre Courbot wrote:
> On Fri May 30, 2025 at 3:22 PM JST, Greg KH wrote:
> > On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
> >> However, Nova also supports a couple of older chip generations that use
> >> the same GSP firmware -  it is for these that the ELF unpacking must
> >> occur in the kernel. IIUC this has to do with the capabilities of the
> >> microcontroller that ultimately does the loading (more capable RISC-V on
> >> Hopper+ vs. older and more limited Falcon).
> >
> > Why specifically does the kernel have to get involved here?  What
> > requires it to do it that userspace can not?
> 
> I don't know of a user-space tool that is readily available and could
> perform such extraction of the ELF content upon kernel request. Is there
> anything like this?

libelf provides you with the needed tools for this.

And you didn't answer my question.

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

On Fri May 30, 2025 at 6:01 PM JST, Greg KH wrote:
> On Fri, May 30, 2025 at 03:59:03PM +0900, Alexandre Courbot wrote:
>> On Fri May 30, 2025 at 3:22 PM JST, Greg KH wrote:
>> > On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
>> >> However, Nova also supports a couple of older chip generations that use
>> >> the same GSP firmware -  it is for these that the ELF unpacking must
>> >> occur in the kernel. IIUC this has to do with the capabilities of the
>> >> microcontroller that ultimately does the loading (more capable RISC-V on
>> >> Hopper+ vs. older and more limited Falcon).
>> >
>> > Why specifically does the kernel have to get involved here?  What
>> > requires it to do it that userspace can not?
>> 
>> I don't know of a user-space tool that is readily available and could
>> perform such extraction of the ELF content upon kernel request. Is there
>> anything like this?
>
> libelf provides you with the needed tools for this.
>
> And you didn't answer my question.

Yes, extracting a section of an ELF file is as trivial as calling
objcopy, no issue with that.

What I don't understand is, who calls objcopy to do that in the first
place, when, and how is the extracted section passed to the kernel?
After digging a bit I found out about CONFIG_FW_LOADER_USER_HELPER which
looks like it could help, but that option is disabled even on my Arch
stock kernel.

But even assuming it was readily available, how to use it is not clear
to me and I could not find a single actual example. I assumed a udev
rule could catch the uevent and call a script that extracts the section
and load it through the sysfs loading interface, but
fallback-mechanisms.rst mentions that "...however firmware loading
support was removed from udev as of systemd commit be2ea723b1d0". Which
makes this idea look like a dead-end.

So to try to answer your question, I am not disagreeing that userspace
is capable of doing what we currently do in the kernel. My follow-up
questions to that are: how do we command userspace to do that work for
us when we request the firmware, how do we provide the result to the
kernel, and is this something that distros can adopt easily? I'm happy
to consider doing things this way, but would need a few pointers to look
into.

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
> On Fri May 30, 2025 at 6:01 PM JST, Greg KH wrote:
> > On Fri, May 30, 2025 at 03:59:03PM +0900, Alexandre Courbot wrote:
> >> On Fri May 30, 2025 at 3:22 PM JST, Greg KH wrote:
> >> > On Fri, May 30, 2025 at 09:58:02AM +0900, Alexandre Courbot wrote:
> >> >> However, Nova also supports a couple of older chip generations that use
> >> >> the same GSP firmware -  it is for these that the ELF unpacking must
> >> >> occur in the kernel. IIUC this has to do with the capabilities of the
> >> >> microcontroller that ultimately does the loading (more capable RISC-V on
> >> >> Hopper+ vs. older and more limited Falcon).
> >> >
> >> > Why specifically does the kernel have to get involved here?  What
> >> > requires it to do it that userspace can not?
> >> 
> >> I don't know of a user-space tool that is readily available and could
> >> perform such extraction of the ELF content upon kernel request. Is there
> >> anything like this?
> >
> > libelf provides you with the needed tools for this.
> >
> > And you didn't answer my question.
> 
> Yes, extracting a section of an ELF file is as trivial as calling
> objcopy, no issue with that.

Great!

> What I don't understand is, who calls objcopy to do that in the first
> place, when, and how is the extracted section passed to the kernel?
> After digging a bit I found out about CONFIG_FW_LOADER_USER_HELPER which
> looks like it could help, but that option is disabled even on my Arch
> stock kernel.

Yes, userspace is the thing that does this when it is told to do it by
the kernel.

> But even assuming it was readily available, how to use it is not clear
> to me and I could not find a single actual example. I assumed a udev
> rule could catch the uevent and call a script that extracts the section
> and load it through the sysfs loading interface, but
> fallback-mechanisms.rst mentions that "...however firmware loading
> support was removed from udev as of systemd commit be2ea723b1d0". Which
> makes this idea look like a dead-end.

Look at how all firmware is loaded on your system today, this isn't a
new thing, it's been working well for everyone for decades now :)

> So to try to answer your question, I am not disagreeing that userspace
> is capable of doing what we currently do in the kernel. My follow-up
> questions to that are: how do we command userspace to do that work for
> us when we request the firmware, how do we provide the result to the
> kernel, and is this something that distros can adopt easily? I'm happy
> to consider doing things this way, but would need a few pointers to look
> into.

Again, look at how your firmware for your devices in your laptop are
loaded today.

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Timur Tabi]

On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
> > So to try to answer your question, I am not disagreeing that userspace
> > is capable of doing what we currently do in the kernel. My follow-up
> > questions to that are: how do we command userspace to do that work for
> > us when we request the firmware, how do we provide the result to the
> > kernel, and is this something that distros can adopt easily? I'm happy
> > to consider doing things this way, but would need a few pointers to look
> > into.
>
> Again, look at how your firmware for your devices in your laptop are
> loaded today.

Today, Nouveau loads and parses these binary images (that are already
in linux-firmware) in the driver.  As I said before, Nova/Nouveau are
using ELF simply as a packaging format, so that these small binary
blobs are kept together and processed as one.  It makes no sense for
Nouveau to consume them as-is, but Nova has to have user-space break
them up first.

We could easily have said that the format is proprietary and not used
the word "elf" in the parser.

IMHO, Nova should really do what Nouveau does, and just have the image
parser in the driver itself, without any generic Rust code to do it.
After all, what Nova needs to do with these images is driver-specific.

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, May 30, 2025 at 01:10:50PM -0500, Timur Tabi wrote:
> On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
> > > So to try to answer your question, I am not disagreeing that userspace
> > > is capable of doing what we currently do in the kernel. My follow-up
> > > questions to that are: how do we command userspace to do that work for
> > > us when we request the firmware, how do we provide the result to the
> > > kernel, and is this something that distros can adopt easily? I'm happy
> > > to consider doing things this way, but would need a few pointers to look
> > > into.
> >
> > Again, look at how your firmware for your devices in your laptop are
> > loaded today.

Note, I am talking about non-gpu firmare images here (wifi, usb
controllers, etc.) that are using the firmware download subsystem for
ages as examples of what to look at as to how to trigger a firmware
image to be loaded by userspace into the device.

> Today, Nouveau loads and parses these binary images (that are already
> in linux-firmware) in the driver.  As I said before, Nova/Nouveau are
> using ELF simply as a packaging format, so that these small binary
> blobs are kept together and processed as one.  It makes no sense for
> Nouveau to consume them as-is, but Nova has to have user-space break
> them up first.
> 
> We could easily have said that the format is proprietary and not used
> the word "elf" in the parser.

And even if you did that, I would say "do it in userspace as firmware
images should be pass-through only".

> IMHO, Nova should really do what Nouveau does, and just have the image
> parser in the driver itself, without any generic Rust code to do it.
> After all, what Nova needs to do with these images is driver-specific.

Again, no, do not do any firmware image parsing in the kernel please
unless you can prove exactly why it MUST be done there.

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Timur Tabi]

On Sat, May 31, 2025 at 12:45 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> > IMHO, Nova should really do what Nouveau does, and just have the image
> > parser in the driver itself, without any generic Rust code to do it.
> > After all, what Nova needs to do with these images is driver-specific.
>
> Again, no, do not do any firmware image parsing in the kernel please
> unless you can prove exactly why it MUST be done there.

Nouveau is already doing all this, just in C.  This entire argument is
over a 12-line function:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c#n1824

Nouveau needs to do this in kernel space because, once it finds the
appropriate section in the ELF image, it reads a hardware register to
determine how to patch it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c#n342

Since this hardware register cannot be read in user-space, this needs
to be done in the kernel.

Please note that there other drivers in Linux that iterate over ELF
sections in order to parse their firmware images:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/imagination/pvr_fw_util.c#n29
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c#n925
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c#n1374

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Sat, May 31, 2025 at 05:17:48AM -0500, Timur Tabi wrote:
> On Sat, May 31, 2025 at 12:45 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > > IMHO, Nova should really do what Nouveau does, and just have the image
> > > parser in the driver itself, without any generic Rust code to do it.
> > > After all, what Nova needs to do with these images is driver-specific.
> >
> > Again, no, do not do any firmware image parsing in the kernel please
> > unless you can prove exactly why it MUST be done there.
> 
> Nouveau is already doing all this, just in C.  This entire argument is
> over a 12-line function:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c#n1824
> 
> Nouveau needs to do this in kernel space because, once it finds the
> appropriate section in the ELF image, it reads a hardware register to
> determine how to patch it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nvkm/falcon/gm200.c#n342
> 
> Since this hardware register cannot be read in user-space, this needs
> to be done in the kernel.

What exactly do you mean by this?  That is what I have been asking, what
is the specific reason why this can't be done in userspace?  What
hardware "thing" can't be read by userspace, and why not?  Userspace has
access to PCI devices directly, surely there is nothing "secret" here.

> Please note that there other drivers in Linux that iterate over ELF
> sections in order to parse their firmware images:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/imagination/pvr_fw_util.c#n29
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c#n925
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c#n1374

As pointed out before, those have "slipped in" and should not be used to
justify continuing to do the same thing.

Again, just do it in userspace, if it's "just" 12 lines in the kernel,
then put those 12 lines in userspace and you are fine.

And the proposed patch was NOT 12 lines of rust, so please don't
conflate the two things here.  That's not what we are talking about.

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

Hi Greg,

On Sat May 31, 2025 at 2:45 PM JST, Greg KH wrote:
> On Fri, May 30, 2025 at 01:10:50PM -0500, Timur Tabi wrote:
>> On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>> >
>> > On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
>> > > So to try to answer your question, I am not disagreeing that userspace
>> > > is capable of doing what we currently do in the kernel. My follow-up
>> > > questions to that are: how do we command userspace to do that work for
>> > > us when we request the firmware, how do we provide the result to the
>> > > kernel, and is this something that distros can adopt easily? I'm happy
>> > > to consider doing things this way, but would need a few pointers to look
>> > > into.
>> >
>> > Again, look at how your firmware for your devices in your laptop are
>> > loaded today.
>
> Note, I am talking about non-gpu firmare images here (wifi, usb
> controllers, etc.) that are using the firmware download subsystem for
> ages as examples of what to look at as to how to trigger a firmware
> image to be loaded by userspace into the device.

I would really appreciate it if you could point me precisely to one
example (a link, a function, a file) of what you are describing because
I'm starting to wonder whether we are talking about the same thing.

Previously I mentioned udev and CONFIG_FW_LOADER_USER_HELPER, but you
haven't confirmed whether that was what you had in mind or not. Assuming
that udev is involved, I tried to snoop events while a
`request_firwmare` call is performed using `udevadm monitor`, but that
revealed no event related to firmware loading. Then looking deeper into
the kernel documentation confirmed that the kernel does indeed a direct
filesystem lookup in request_firmware [1]. IOW, the kernel looks for the
requested file, and if it cannot find it it's game over. This matches my
observations with udevadm, as I tried requesting a non-existing file and
no uevent was generated. I don't see what user-space can do here.

I also tried to look up this "firmware download subsystem" you
mentioned, but couldn't find anything under that name - I suspect you
are talking about the sysfs loading mechanism, but AFAIU this depends on 
CONFIG_FW_LOADER_USER_HELPER which doesn't seem to be widely enabled
(not on my distro at least).

What I am trying to say is that I am more than willing to look into
having this work done by user-space, but despite this conversation and
hours of research and testing I still don't have even a thread to
unravel. If you know the answer for how to do this, please share it.

[1] https://docs.kernel.org/driver-api/firmware/direct-fs-lookup.html

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Sat, May 31, 2025 at 09:33:38PM +0900, Alexandre Courbot wrote:
> Hi Greg,
> 
> On Sat May 31, 2025 at 2:45 PM JST, Greg KH wrote:
> > On Fri, May 30, 2025 at 01:10:50PM -0500, Timur Tabi wrote:
> >> On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >
> >> > On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
> >> > > So to try to answer your question, I am not disagreeing that userspace
> >> > > is capable of doing what we currently do in the kernel. My follow-up
> >> > > questions to that are: how do we command userspace to do that work for
> >> > > us when we request the firmware, how do we provide the result to the
> >> > > kernel, and is this something that distros can adopt easily? I'm happy
> >> > > to consider doing things this way, but would need a few pointers to look
> >> > > into.
> >> >
> >> > Again, look at how your firmware for your devices in your laptop are
> >> > loaded today.
> >
> > Note, I am talking about non-gpu firmare images here (wifi, usb
> > controllers, etc.) that are using the firmware download subsystem for
> > ages as examples of what to look at as to how to trigger a firmware
> > image to be loaded by userspace into the device.
> 
> I would really appreciate it if you could point me precisely to one
> example (a link, a function, a file) of what you are describing because
> I'm starting to wonder whether we are talking about the same thing.
> 
> Previously I mentioned udev and CONFIG_FW_LOADER_USER_HELPER, but you
> haven't confirmed whether that was what you had in mind or not. Assuming
> that udev is involved, I tried to snoop events while a
> `request_firwmare` call is performed using `udevadm monitor`, but that
> revealed no event related to firmware loading. Then looking deeper into
> the kernel documentation confirmed that the kernel does indeed a direct
> filesystem lookup in request_firmware [1]. IOW, the kernel looks for the
> requested file, and if it cannot find it it's game over. This matches my
> observations with udevadm, as I tried requesting a non-existing file and
> no uevent was generated. I don't see what user-space can do here.
> 
> I also tried to look up this "firmware download subsystem" you
> mentioned, but couldn't find anything under that name - I suspect you
> are talking about the sysfs loading mechanism, but AFAIU this depends on 
> CONFIG_FW_LOADER_USER_HELPER which doesn't seem to be widely enabled
> (not on my distro at least).

Yes, that is what I am referring to, as you all seem to want to do
"complex things without a specific filename choosen".  Look at
Documentation/driver-api/firmware/fallback-mechanisms.rst for the
details there.

Or, better yet, just have your driver name all of the individual files
that must be loaded and then no userspace things are needed.  That "big"
firmware file will have already been split up into the different parts
when you write it out to the filesystem, so no need to parse anything.

If this isn't going to work for some reason, I think we need a better
"this is EXACTLY what we need to send to the hardware for the firmware
image(s) it requires" as I'm totally confused based on the different
people talking on this thread about totally different hypotheticals
(i.e. 12 line elf parsers in C vs. a giant elf parser in rust, random
hypothetical hardware values that userspace "can not know", pointing at
obsolete crazy interfaces like remoteproc that just happen to do crazy
things, etc.)

So step back, come up with a solid design document, and let's start over
please.

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Timur Tabi]

On Sat, May 31, 2025 at 7:25 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> What exactly do you mean by this?  That is what I have been asking, what
> is the specific reason why this can't be done in userspace?  What
> hardware "thing" can't be read by userspace, and why not?  Userspace has
> access to PCI devices directly, surely there is nothing "secret" here.

Why in the world would you want user space to read hardware registers,
when the driver is already doing it???????

And please note that the driver has to read and parse a lot of other
register in order to know which register contains the fuse settings,
and even whether that register exists, and at what address.  The fuse
register is hardware-specific.  It doesn't exist on Turing, it does
exist on Ampere and Ada (but just GA10x, not GA100), and it's not used
on Hopper and Blackwell.  You want to duplicate all this code in
user-space (assuming that registers really are accessible in user
space), just avoid a 12-line function that already exists and works in
Nouveau?????????

Please make it make sense, Greg.

> > Please note that there other drivers in Linux that iterate over ELF
> > sections in order to parse their firmware images:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/imagination/pvr_fw_util.c#n29
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/platform/st/sti/c8sectpfe/c8sectpfe-core.c#n925
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/remoteproc/qcom_q6v5_mss.c#n1374
>
> As pointed out before, those have "slipped in" and should not be used to
> justify continuing to do the same thing.
>
> Again, just do it in userspace, if it's "just" 12 lines in the kernel,
> then put those 12 lines in userspace and you are fine.

Those 12 lines are used to determine how to patch the image.  We would
need to move all that patching code and all the GPU detection code, as
well as the list of all the relevant GPU registers into user-space.

> And the proposed patch was NOT 12 lines of rust, so please don't
> conflate the two things here.  That's not what we are talking about.

We can easily strip it down to the bare minimum and keep it private to
Nova, if you want.  After all, that's how Nouveau does this.

Re: [PATCH] rust: add basic ELF sections parser

[Danilo Krummrich]

On Sat, May 31, 2025 at 09:38:24AM -0500, Timur Tabi wrote:
> On Sat, May 31, 2025 at 7:25 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > What exactly do you mean by this?  That is what I have been asking, what
> > is the specific reason why this can't be done in userspace?  What
> > hardware "thing" can't be read by userspace, and why not?  Userspace has
> > access to PCI devices directly, surely there is nothing "secret" here.
> 
> Why in the world would you want user space to read hardware registers,
> when the driver is already doing it???????

You're probably talking past each other; maybe Greg was thinking of information
that is also accessible via sysfs, e.g. configuration space registers, etc.

Anyways, I also suggest to take a step back and write up a few lines that
provide a slightly broader context how that process of loading the firmware
works and what the technical requirements are.

It feels like the discussion is missing a common understanding of what are the
exact problems that should be solved here -- let's get this common understanding
first and then let's figure out if the idea of parsing the full ELF image is the
correct solution, is part of the correct solution or whether it can be solved
differently as well.

--

Like I mentioned previously, there's no need to make this discussion a blocker
for progressing nova-core. If we have to, I'm fine to apply the required code
in nova-core in the meantime.

But at the same time I also want to make sure that we discussed this - with a
common understanding of the problem - and find consensus on what's the correct
thing to solve this properly.

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Sat, May 31, 2025 at 09:38:24AM -0500, Timur Tabi wrote:
> On Sat, May 31, 2025 at 7:25 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > What exactly do you mean by this?  That is what I have been asking, what
> > is the specific reason why this can't be done in userspace?  What
> > hardware "thing" can't be read by userspace, and why not?  Userspace has
> > access to PCI devices directly, surely there is nothing "secret" here.
> 
> Why in the world would you want user space to read hardware registers,
> when the driver is already doing it???????
> 
> And please note that the driver has to read and parse a lot of other
> register in order to know which register contains the fuse settings,
> and even whether that register exists, and at what address.  The fuse
> register is hardware-specific.  It doesn't exist on Turing, it does
> exist on Ampere and Ada (but just GA10x, not GA100), and it's not used
> on Hopper and Blackwell.  You want to duplicate all this code in
> user-space (assuming that registers really are accessible in user
> space), just avoid a 12-line function that already exists and works in
> Nouveau?????????

Because you are not proposing a 12 line function here, you all were
attempting to add an elf parser to the core kernel that everyone would
always load into their memory space.

> Please make it make sense, Greg.

Please read my thread where I asked for detailed specifics as to exactly
what is required here for you all to determine what is needed to load
the firmware.

have a good weekend,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

Hi Greg,

On Sat May 31, 2025 at 10:30 PM JST, Greg KH wrote:
> On Sat, May 31, 2025 at 09:33:38PM +0900, Alexandre Courbot wrote:
>> Hi Greg,
>> 
>> On Sat May 31, 2025 at 2:45 PM JST, Greg KH wrote:
>> > On Fri, May 30, 2025 at 01:10:50PM -0500, Timur Tabi wrote:
>> >> On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> >
>> >> > On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
>> >> > > So to try to answer your question, I am not disagreeing that userspace
>> >> > > is capable of doing what we currently do in the kernel. My follow-up
>> >> > > questions to that are: how do we command userspace to do that work for
>> >> > > us when we request the firmware, how do we provide the result to the
>> >> > > kernel, and is this something that distros can adopt easily? I'm happy
>> >> > > to consider doing things this way, but would need a few pointers to look
>> >> > > into.
>> >> >
>> >> > Again, look at how your firmware for your devices in your laptop are
>> >> > loaded today.
>> >
>> > Note, I am talking about non-gpu firmare images here (wifi, usb
>> > controllers, etc.) that are using the firmware download subsystem for
>> > ages as examples of what to look at as to how to trigger a firmware
>> > image to be loaded by userspace into the device.
>> 
>> I would really appreciate it if you could point me precisely to one
>> example (a link, a function, a file) of what you are describing because
>> I'm starting to wonder whether we are talking about the same thing.
>> 
>> Previously I mentioned udev and CONFIG_FW_LOADER_USER_HELPER, but you
>> haven't confirmed whether that was what you had in mind or not. Assuming
>> that udev is involved, I tried to snoop events while a
>> `request_firwmare` call is performed using `udevadm monitor`, but that
>> revealed no event related to firmware loading. Then looking deeper into
>> the kernel documentation confirmed that the kernel does indeed a direct
>> filesystem lookup in request_firmware [1]. IOW, the kernel looks for the
>> requested file, and if it cannot find it it's game over. This matches my
>> observations with udevadm, as I tried requesting a non-existing file and
>> no uevent was generated. I don't see what user-space can do here.
>> 
>> I also tried to look up this "firmware download subsystem" you
>> mentioned, but couldn't find anything under that name - I suspect you
>> are talking about the sysfs loading mechanism, but AFAIU this depends on 
>> CONFIG_FW_LOADER_USER_HELPER which doesn't seem to be widely enabled
>> (not on my distro at least).
>
> Yes, that is what I am referring to, as you all seem to want to do
> "complex things without a specific filename choosen".  Look at
> Documentation/driver-api/firmware/fallback-mechanisms.rst for the
> details there.
>
> Or, better yet, just have your driver name all of the individual files
> that must be loaded and then no userspace things are needed.  That "big"
> firmware file will have already been split up into the different parts
> when you write it out to the filesystem, so no need to parse anything.
>
> If this isn't going to work for some reason, I think we need a better
> "this is EXACTLY what we need to send to the hardware for the firmware
> image(s) it requires" as I'm totally confused based on the different
> people talking on this thread about totally different hypotheticals
> (i.e. 12 line elf parsers in C vs. a giant elf parser in rust, random
> hypothetical hardware values that userspace "can not know", pointing at
> obsolete crazy interfaces like remoteproc that just happen to do crazy
> things, etc.)
>
> So step back, come up with a solid design document, and let's start over
> please.

Sounds good. Let me finish evaluating all the alternatives, gather the
pros/cons of each option, and lay out the context for our needs as
clearly as possible - this should take a few days.

Cheers,
Alex.

Re: [PATCH] rust: add basic ELF sections parser

[Alexandre Courbot]

Hi Greg,

On Sat May 31, 2025 at 10:30 PM JST, Greg KH wrote:
> On Sat, May 31, 2025 at 09:33:38PM +0900, Alexandre Courbot wrote:
>> Hi Greg,
>> 
>> On Sat May 31, 2025 at 2:45 PM JST, Greg KH wrote:
>> > On Fri, May 30, 2025 at 01:10:50PM -0500, Timur Tabi wrote:
>> >> On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>> >> >
>> >> > On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
>> >> > > So to try to answer your question, I am not disagreeing that userspace
>> >> > > is capable of doing what we currently do in the kernel. My follow-up
>> >> > > questions to that are: how do we command userspace to do that work for
>> >> > > us when we request the firmware, how do we provide the result to the
>> >> > > kernel, and is this something that distros can adopt easily? I'm happy
>> >> > > to consider doing things this way, but would need a few pointers to look
>> >> > > into.
>> >> >
>> >> > Again, look at how your firmware for your devices in your laptop are
>> >> > loaded today.
>> >
>> > Note, I am talking about non-gpu firmare images here (wifi, usb
>> > controllers, etc.) that are using the firmware download subsystem for
>> > ages as examples of what to look at as to how to trigger a firmware
>> > image to be loaded by userspace into the device.
>> 
>> I would really appreciate it if you could point me precisely to one
>> example (a link, a function, a file) of what you are describing because
>> I'm starting to wonder whether we are talking about the same thing.
>> 
>> Previously I mentioned udev and CONFIG_FW_LOADER_USER_HELPER, but you
>> haven't confirmed whether that was what you had in mind or not. Assuming
>> that udev is involved, I tried to snoop events while a
>> `request_firwmare` call is performed using `udevadm monitor`, but that
>> revealed no event related to firmware loading. Then looking deeper into
>> the kernel documentation confirmed that the kernel does indeed a direct
>> filesystem lookup in request_firmware [1]. IOW, the kernel looks for the
>> requested file, and if it cannot find it it's game over. This matches my
>> observations with udevadm, as I tried requesting a non-existing file and
>> no uevent was generated. I don't see what user-space can do here.
>> 
>> I also tried to look up this "firmware download subsystem" you
>> mentioned, but couldn't find anything under that name - I suspect you
>> are talking about the sysfs loading mechanism, but AFAIU this depends on 
>> CONFIG_FW_LOADER_USER_HELPER which doesn't seem to be widely enabled
>> (not on my distro at least).
>
> Yes, that is what I am referring to, as you all seem to want to do
> "complex things without a specific filename choosen".  Look at
> Documentation/driver-api/firmware/fallback-mechanisms.rst for the
> details there.
>
> Or, better yet, just have your driver name all of the individual files
> that must be loaded and then no userspace things are needed.  That "big"
> firmware file will have already been split up into the different parts
> when you write it out to the filesystem, so no need to parse anything.
>
> If this isn't going to work for some reason, I think we need a better
> "this is EXACTLY what we need to send to the hardware for the firmware
> image(s) it requires" as I'm totally confused based on the different
> people talking on this thread about totally different hypotheticals
> (i.e. 12 line elf parsers in C vs. a giant elf parser in rust, random
> hypothetical hardware values that userspace "can not know", pointing at
> obsolete crazy interfaces like remoteproc that just happen to do crazy
> things, etc.)
>
> So step back, come up with a solid design document, and let's start over
> please.
>
> thanks,
>
> greg k-h

Sorry for the time it took to come back to you on this.

After further investigation, it appears that most of the points we
discussed were indeed orthogonal to whether we rely on ELF or the
filesystem to provide the different parts of the firmware, so I'd like
to apologize for the unneeded confusion.

We had an internal discussion with our firmware team about future
firmware releases. As it turns out, the firmware itself is undergoing an
overhaul, so we would like to take that opportunity to re-think the
release format and try to address this issue.

It will likely take a few months to reach a definitive design, and in
the meantime we would like to keep making progress on bringing up Nova
with the currently released firmware images. However, since Nova is
still in early development, we don't need to maintain long-term support
for these specific images.

Based on that, I would like to proceed as follows:

- Ask Danilo to include a stripped down (<30 LoC without comments) and
  temporary version of the ELF section unpacker in nova-core so we can
  use the images currently in linux-firmware for short-term development.
  I want to stress that this is temporary, and stable Nova will *not*
  support these images; this is solely to enable us to move forward for
  the time being.
- We (NVIDIA folks involved in Nova) will continue working with the
  firmware team to ensure that the upcoming redesign takes into account
  the kernel's requirements, especially the need to avoid unnecessary
  complexity in the firmware loading steps. There are other constraints
  of course (the hardware itself continues to evolve, with consequences
  for the firmware), and so we may or may not achieve everything we hope
  for. But we will work to keep it as simple as possible.
- Once a stable firmware ABI is established and its first instance
  released, we will make it the minimum supported firmware version on
  Nova and remove the band-aid mentioned in the first point.
- If willing, Danilo will keep us honest on all this. :)

I hope this sounds good to you.

Cheers,
Alex.

Re: [PATCH] rust: add basic ELF sections parser

[Greg KH]

On Fri, Jun 13, 2025 at 12:32:13PM +0900, Alexandre Courbot wrote:
> Hi Greg,
> 
> On Sat May 31, 2025 at 10:30 PM JST, Greg KH wrote:
> > On Sat, May 31, 2025 at 09:33:38PM +0900, Alexandre Courbot wrote:
> >> Hi Greg,
> >> 
> >> On Sat May 31, 2025 at 2:45 PM JST, Greg KH wrote:
> >> > On Fri, May 30, 2025 at 01:10:50PM -0500, Timur Tabi wrote:
> >> >> On Fri, May 30, 2025 at 10:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >> >> >
> >> >> > On Fri, May 30, 2025 at 11:34:02PM +0900, Alexandre Courbot wrote:
> >> >> > > So to try to answer your question, I am not disagreeing that userspace
> >> >> > > is capable of doing what we currently do in the kernel. My follow-up
> >> >> > > questions to that are: how do we command userspace to do that work for
> >> >> > > us when we request the firmware, how do we provide the result to the
> >> >> > > kernel, and is this something that distros can adopt easily? I'm happy
> >> >> > > to consider doing things this way, but would need a few pointers to look
> >> >> > > into.
> >> >> >
> >> >> > Again, look at how your firmware for your devices in your laptop are
> >> >> > loaded today.
> >> >
> >> > Note, I am talking about non-gpu firmare images here (wifi, usb
> >> > controllers, etc.) that are using the firmware download subsystem for
> >> > ages as examples of what to look at as to how to trigger a firmware
> >> > image to be loaded by userspace into the device.
> >> 
> >> I would really appreciate it if you could point me precisely to one
> >> example (a link, a function, a file) of what you are describing because
> >> I'm starting to wonder whether we are talking about the same thing.
> >> 
> >> Previously I mentioned udev and CONFIG_FW_LOADER_USER_HELPER, but you
> >> haven't confirmed whether that was what you had in mind or not. Assuming
> >> that udev is involved, I tried to snoop events while a
> >> `request_firwmare` call is performed using `udevadm monitor`, but that
> >> revealed no event related to firmware loading. Then looking deeper into
> >> the kernel documentation confirmed that the kernel does indeed a direct
> >> filesystem lookup in request_firmware [1]. IOW, the kernel looks for the
> >> requested file, and if it cannot find it it's game over. This matches my
> >> observations with udevadm, as I tried requesting a non-existing file and
> >> no uevent was generated. I don't see what user-space can do here.
> >> 
> >> I also tried to look up this "firmware download subsystem" you
> >> mentioned, but couldn't find anything under that name - I suspect you
> >> are talking about the sysfs loading mechanism, but AFAIU this depends on 
> >> CONFIG_FW_LOADER_USER_HELPER which doesn't seem to be widely enabled
> >> (not on my distro at least).
> >
> > Yes, that is what I am referring to, as you all seem to want to do
> > "complex things without a specific filename choosen".  Look at
> > Documentation/driver-api/firmware/fallback-mechanisms.rst for the
> > details there.
> >
> > Or, better yet, just have your driver name all of the individual files
> > that must be loaded and then no userspace things are needed.  That "big"
> > firmware file will have already been split up into the different parts
> > when you write it out to the filesystem, so no need to parse anything.
> >
> > If this isn't going to work for some reason, I think we need a better
> > "this is EXACTLY what we need to send to the hardware for the firmware
> > image(s) it requires" as I'm totally confused based on the different
> > people talking on this thread about totally different hypotheticals
> > (i.e. 12 line elf parsers in C vs. a giant elf parser in rust, random
> > hypothetical hardware values that userspace "can not know", pointing at
> > obsolete crazy interfaces like remoteproc that just happen to do crazy
> > things, etc.)
> >
> > So step back, come up with a solid design document, and let's start over
> > please.
> >
> > thanks,
> >
> > greg k-h
> 
> Sorry for the time it took to come back to you on this.
> 
> After further investigation, it appears that most of the points we
> discussed were indeed orthogonal to whether we rely on ELF or the
> filesystem to provide the different parts of the firmware, so I'd like
> to apologize for the unneeded confusion.
> 
> We had an internal discussion with our firmware team about future
> firmware releases. As it turns out, the firmware itself is undergoing an
> overhaul, so we would like to take that opportunity to re-think the
> release format and try to address this issue.
> 
> It will likely take a few months to reach a definitive design, and in
> the meantime we would like to keep making progress on bringing up Nova
> with the currently released firmware images. However, since Nova is
> still in early development, we don't need to maintain long-term support
> for these specific images.
> 
> Based on that, I would like to proceed as follows:
> 
> - Ask Danilo to include a stripped down (<30 LoC without comments) and
>   temporary version of the ELF section unpacker in nova-core so we can
>   use the images currently in linux-firmware for short-term development.
>   I want to stress that this is temporary, and stable Nova will *not*
>   support these images; this is solely to enable us to move forward for
>   the time being.

We know no user/kernel api can ever be changed in the future, so how can
we ever accept this?

> - We (NVIDIA folks involved in Nova) will continue working with the
>   firmware team to ensure that the upcoming redesign takes into account
>   the kernel's requirements, especially the need to avoid unnecessary
>   complexity in the firmware loading steps. There are other constraints
>   of course (the hardware itself continues to evolve, with consequences
>   for the firmware), and so we may or may not achieve everything we hope
>   for. But we will work to keep it as simple as possible.

Great, we can wait for that work to be done, we have no deadlines or
rush here to do it right.  Again, once we add a user/kernel api, we
can't remove it, so let's take the time to get it right.

> - Once a stable firmware ABI is established and its first instance
>   released, we will make it the minimum supported firmware version on
>   Nova and remove the band-aid mentioned in the first point.

Don't you always just move the firmware version forward on new updates
to the kernel driver anyway?  How is this different from normal
operation except that the user/kernel api will now be changed in a
non-backwards-compatible way.  What will break when this happens?

thanks,

greg k-h

Re: [PATCH] rust: add basic ELF sections parser

[Danilo Krummrich]

(Cc: +Dave)

On Tue, Jun 24, 2025 at 03:26:11PM +0100, Greg KH wrote:
> We know no user/kernel api can ever be changed in the future, so how can
> we ever accept this?

I think the nova project is a bit special in this aspect, since we develop the
driver upstream for the purpose of breaking the chicken-egg problem of not
getting infrastructure upstream while not having a user, but not getting a user
upstream without having the corresponding infrastructure.

Given that, neither nova-core nor nova-drm make any promises to be working
drivers in the sense of being useful to users.

However, eventually we will hit the point where enough development has been done
such that the drivers actually become useful for users. This will be the turning
point at which we will of course guarantee to keep supporting a firmware version
(or any other uAPI) once introduced.

Until then, it is what the Kconfig claims: "This driver is work in progress and
may not be functional."