Message-Id: <D9XNS413TVXB.3SWWJE4JGEN8B@nvidia.com>
Date: Fri, 16 May 2025 23:35:42 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "Greg KH" <gregkh@...uxfoundation.org>, "John Hubbard"
 <jhubbard@...dia.com>, "Timur Tabi" <timur@...nel.org>, "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 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@...uxfoundation.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.

Powered by blists - more mailing lists