| 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
| ||
|
Message-Id: <DCOYNWXYKBOK.XCRA4Z34RUXP@kernel.org>
Date: Wed, 10 Sep 2025 10:01:22 +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 10, 2025 at 6:48 AM CEST, Alexandre Courbot wrote:
> On Tue Sep 9, 2025 at 11:43 PM JST, Danilo Krummrich wrote:
>> impl Gpu {
>> pub(crate) fn new<'a>(
>> dev: &'a Device<Bound>,
>> bar: &'a Bar0
>> devres_bar: Arc<Devres<Bar0>>,
>> ) -> impl PinInit<Self, Error> + 'a {
>> try_pin_init(Self {
>> bar: devres_bar,
>> spec: Spec::new(bar)?,
>> gsp_falcon: Falcon::<Gsp>::new(dev, spec.chipset)?,
>> sec2_falcon: Falcon::<Sec2>::new(dev, spec.chipset)?,
>> sysmem_flush: SysmemFlush::register(dev, bar, spec.chipset)?
>> gsp <- Gsp::new(gsp_falcon, sec2_falcon, sysmem_flush)?,
>> })
>> }
>> }
>
> It does work. The bizareness of passing the `bar` twice aside,
Yeah, but it really seems like a minor inconvinience. (Especially, since this
will be the only occurance of this we'll ever have.)
> here is what it looks like when I got it to compile:
This looks great!
> pub(crate) fn new<'a>(
> pdev: &'a pci::Device<device::Bound>,
> devres_bar: Arc<Devres<Bar0>>,
> bar: &'a Bar0,
> ) -> impl PinInit<Self, Error> + 'a {
> try_pin_init!(Self {
> spec: Spec::new(bar).inspect(|spec| {
> dev_info!(
> pdev.as_ref(),
> "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
> spec.chipset,
> spec.chipset.arch(),
> spec.revision
> );
> })?,
+ _: {
+ gfw::wait_gfw_boot_completion(bar)
+ .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete"))?;
+ },
>
> sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
>
> gsp_falcon: Falcon::<Gsp>::new(
> pdev.as_ref(),
> spec.chipset,
> bar,
> spec.chipset > Chipset::GA100,
> )
> .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>
> sec2_falcon: Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?,
>
- gsp: Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?,
+ gsp <- Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon),
>
> bar: devres_bar,
> })
> }
>
> The wait for GFW initialization had to be moved to `probe`, but that's
> fine IMO.
That's not necessary, you can keep it in Gpu::new() -- I don't see what's
preventing us from that. I inserted it in the code above.
> I do however find the code less readable in this form, less
> editable as well. And LSP seems lost, so I don't get any syntax
> highlighting in the `try_pin_init` block.
Benno is working on a syntax update, so automatic formatting etc. will properly
work.
Otherwise, I can't see how this is a downgrade. It represents the initialization
process in a much clearer way that the current implementation of Gsp::new(),
which is rather messy.
> Fundamentally, this changes the method from a fallible method returning
> a non-fallible initializer into a non-fallible method returning a
> fallible initializer.
Yeah, that's the best case when working with pin-init.
> I'm ok with that, and maybe this new form will
> encourage us to keep this method short, which is what we want, but other
> than that what benefit are we getting from this change?
The immediate benefit is that we don't need an extra allocation for the Gsp
structure.
The general benefit is that once we need to add more fields to
structures that require pinning (such as locks -- and we will need a lot of
them) we're prepared for it.
If we're not prepared for it, I'm pretty sure that everytime someone needs to
add e.g. a new lock for something, it will just result in a new Pin<KBox<T>>,
because the initial pin-init hierarchy just isn't there, and creating a new
allocation is more convinient than fixing the existing code.
This is exactly what pin-init was introduced for in the kernel, such that we're
not doing worse than equivalent C code.
struct Foo {
struct Bar bar;
void *data;
struct mutex data_lock;
}
struct Bar {
void *data;
struct mutex data_lock;
}
In C you can just kmalloc(sizeof(Foo), GFP_KERNEL) and subsequently initialize
all fields.
In Rust I don't want to fall back having to allocate for Bar *and* for Foo
separately.
If there are things to improve with pin-init, let's improve them, but let's not
do worse than C code in this aspect.
Powered by blists - more mailing lists