Message-Id: <DA8G3G918FS4.X8D7PQMT4TGB@nvidia.com>
Date: Thu, 29 May 2025 15:53:42 +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

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
> 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?

Powered by blists - more mailing lists