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

[Greg KH <gregkh@linuxfoundation.org>]

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