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] [thread-next>] [day] [month] [year] [list]

Message-Id: <DCOUK0Z4YV6M.2R0CFE57DY5CR@nvidia.com>
Date: Wed, 10 Sep 2025 13:48:12 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>, "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 Tue Sep 9, 2025 at 11:43 PM JST, Danilo Krummrich wrote:
> On Tue Sep 9, 2025 at 4:11 PM CEST, Alexandre Courbot wrote:
>> On Wed Sep 3, 2025 at 5:27 PM JST, Danilo Krummrich wrote:
>>> On Wed Sep 3, 2025 at 9:10 AM CEST, Alexandre Courbot wrote:
>>>> On Wed Sep 3, 2025 at 8:12 AM JST, Danilo Krummrich wrote:
>>>>> On 9/2/25 4:31 PM, Alexandre Courbot wrote:
>>>>>>       pub(crate) fn new(
>>>>>>           pdev: &pci::Device<device::Bound>,
>>>>>>           devres_bar: Arc<Devres<Bar0>>,
>>>>>
>>>>> The diff is hiding it, but with this patch we should also make sure that this 
>>>>> returns impl PinInit<Self, Error> rather than Result<impl PinInit<Self>.
>>>>>
>>>>> I think this should be possible now.
>>>>
>>>> There is still code that can return errors (falcon creation, etc) - do
>>>> you mean that we should move it into the pin initializer and turn it
>>>> into a `try_pin_init`?
>>>
>>> Yeah, that would be better practice, if it doesn't work out for a good reason
>>> we can also fall back to Result<impl PinInit<Self, Error>, but we should at
>>> least try to avoid it.
>>
>> I tried but could not do it in a way that is satisfying. The problem is
>> that `Gpu::new` receives a `Arc<Devres<Bar0>>`, which we need to
>> `access` in order to do anything useful with it. If we first store it
>> into the `Gpu` structure, then every subsequent member needs to `access`
>> it in its own code block in order to perform their own initialization.
>> This is quite cumbersome.
>>
>> If there is a way to obtain the `Bar0` once after the `bar` member of
>> `Gpu` is initialized, and then use that instance with each remaining
>> member, then that problem would go away but I am not aware of such a
>> thing.
>
> What about this?
>
> 	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, here is
what it looks like when I got it to compile:

    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
                );
            })?,

            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)?,

            bar: devres_bar,
        })
    }

The wait for GFW initialization had to be moved to `probe`, but that's
fine IMO. 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.

Fundamentally, this changes the method from a fallible method returning
a non-fallible initializer into a non-fallible method returning a
fallible initializer. 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?