Message-Id: <DA935OIFBM1H.3CMSHQ46LLG4P@nvidia.com>
Date: Fri, 30 May 2025 09:58:02 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Greg KH" <gregkh@...uxfoundation.org>
Cc: "Danilo Krummrich" <dakr@...nel.org>, "Timur Tabi" <timur@...nel.org>,
 "John Hubbard" <jhubbard@...dia.com>, "Miguel Ojeda" <ojeda@...nel.org>,
 "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>,
 "Gary Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] rust: add basic ELF sections parser

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@...dia.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.

Powered by blists - more mailing lists