linux-kernel - Re: [PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250904152646.GA1967049@joelbox2>
Date: Thu, 4 Sep 2025 11:28:06 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Alexandre Courbot <acourbot@...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 <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	John Hubbard <jhubbard@...dia.com>,
	Alistair Popple <apopple@...dia.com>, Timur Tabi <ttabi@...dia.com>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu`
 constructor

Hi Danilo,

On Wed, Sep 03, 2025 at 04:53:57PM +0200, Danilo Krummrich wrote:
> On Wed Sep 3, 2025 at 2:29 PM CEST, Alexandre Courbot wrote:
> > To be honest I am not completely sure about the best layout yet and will
> > need more visibility to understand whether this is optimal. But
> > considering that we want to run the GSP boot process over a built `Gpu`
> > instance, we cannot store the result of said process inside `Gpu` unless
> > we put it inside e.g. an `Option`. But then the variant will always be
> > `Some` after `probe` returns, and yet we will have to perform a match
> > every time we want to access it.
> >
> > The current separation sounds reasonable to me for the time being, with
> > `Gpu` containing purely hardware resources obtained without help from
> > user-space, while `Gsp` is the result of running a bunch of firmwares.
> > An alternative design would be to store `Gpu` inside `Gsp`, but `Gsp`
> > inside `Gpu` is trickier due to the build order. No matter what we do,
> > switching the layout later should be trivial if we don't choose the
> > best one now.
> 
> Gsp should be part of the Gpu object.

Just checking, if Gsp is a part of Gpu as you mentioned, and start_gsp() is
called from within Gpu::new(), does that not avoid the problem of referring
to fields of pin-initialized structs entirely, at least for this usecase for
nova?  Or is there any nova-related usecase where this is problematic?

thanks,

 - Joel



[...]

> The Gpu object represents the entire
> instance of the Gpu, including hardware ressources, firmware runtime state, etc.
> 
> The initialization of the Gsp structure doesn't really need a Gpu structure to
> be constructed, it needs certain members of the Gpu structure, i.e. order of
> initialization of the members does matter.
> 
> If it makes things more obvious we can always create new types and increase the
> hierarchy within the Gpu struct itself.
> 
> The technical limitation you're facing is always the same, no matter the layout
> we choose: we need pin-init to provide us references to already initialized
> members.
> 
> I will check with Benno in today's Rust call what's the best way to address
> this.
> 
> > There is also an easy workaround to the sibling initialization issue,
> > which is to store `Gpu` and `Gsp` behind `Pin<KBox>` - that way we can
> > initialize both outside `try_pin_init!`, at the cost of two more heap
> > allocations over the whole lifetime of the device. If we don't have a
> > proper solution to the problem now, this might be better than using
> > `unsafe` as a temporary solution.
> 
> Yeah, this workaround is much easier to implement when they're siblings (less
> allocations temporarily), but let's not design things this way because of that.
> 
> As mentioned above, I will check with Benno today.
> 
> > The same workaround could also be used for to `GspFirmware` and its page
> > tables - since `GspFirmware` is temporary and can apparently be
> > discarded after the GSP is booted, this shouldn't be a big issue. This
> > will allow the driver to probe, and we can add TODO items to fix that
> > later if a solution is in sight.
> >
> >>
> >> I thought the intent was to keep temporary values local to start_gsp() and not
> >> store them next to Gpu in the same allocation?
> >
> > It is not visible in the current patchset, but `start_gsp` will
> > eventually return the runtime data of the GSP - notably its log buffers
> > and command queue, which are needed to operate it. All the rest (notably
> > the loaded firmwares) will be local to `start_gsp` and discarded upon
> > its return.
> 
> Ok, that makes sense, but it should really be part of the Gpu structure.