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 for Android: free password hash cracker in your pocket

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

Message-Id: <DCJ9206YBEV2.1ICN4VILLM09J@kernel.org>
Date: Wed, 03 Sep 2025 16:53:57 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alexandre Courbot" <acourbot@...dia.com>
Cc: "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>, "Joel
 Fernandes" <joelagnelf@...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

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