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

[Greg KH <gregkh@linuxfoundation.org>]

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