linux-kernel - Re: [PATCH v3 13/19] gpu: nova-core: add falcon register definitions and base code

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: <aCcu_42cM2c-Koxu@pollux>
Date: Fri, 16 May 2025 14:26:39 +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 <benno.lossin@...ton.me>,
	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>,
	Jonathan Corbet <corbet@....net>,
	John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>,
	Joel Fernandes <joelagnelf@...dia.com>,
	Timur Tabi <ttabi@...dia.com>, Alistair Popple <apopple@...dia.com>,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v3 13/19] gpu: nova-core: add falcon register definitions
 and base code

On Fri, May 16, 2025 at 09:19:45PM +0900, Alexandre Courbot wrote:
> On Wed May 14, 2025 at 1:19 AM JST, Danilo Krummrich wrote:
> <snip>
> >> +        util::wait_on(Duration::from_millis(20), || {
> >> +            let r = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, E::BASE);
> >> +            if r.mem_scrubbing() {
> >> +                Some(())
> >> +            } else {
> >> +                None
> >> +            }
> >> +        })
> >> +    }
> >> +
> >> +    /// Reset the falcon engine.
> >> +    fn reset_eng(&self, bar: &Bar0) -> Result<()> {
> >> +        let _ = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, E::BASE);
> >> +
> >> +        // According to OpenRM's `kflcnPreResetWait_GA102` documentation, HW sometimes does not set
> >> +        // RESET_READY so a non-failing timeout is used.
> >
> > Should we still warn about it?
> 
> OpenRM does not (as this is apparently a workaround to a HW bug?) so I
> don't think we need to.
> 
> >
> >> +        let _ = util::wait_on(Duration::from_micros(150), || {
> >
> > Do we know for sure that if RESET_READY is not set after 150us, it won't ever be
> > set? If the answer to that is yes, and we also do not want to warn about
> > RESET_READY not being set, why even bother trying to read it in the first place?
> 
> My guess is because this would the expected behavior if the bug wasn't
> there. My GPU (Ampere) does wait until the timeout, but we can expect
> newer GPUs to not have this problem and return earlier.

Ok, let's keep it then.

> >
> >> +            let r = regs::NV_PFALCON_FALCON_HWCFG2::read(bar, E::BASE);
> >> +            if r.reset_ready() {
> >> +                Some(())
> >> +            } else {
> >> +                None
> >> +            }
> >> +        });
> >> +
> >> +        regs::NV_PFALCON_FALCON_ENGINE::alter(bar, E::BASE, |v| v.set_reset(true));
> >> +
> >> +        let _: Result<()> = util::wait_on(Duration::from_micros(10), || None);
> >
> > Can we please get an abstraction for udelay() for this?
> 
> Should it be local to nova-core, or be generally available? I refrained
> from doing this because there is work going on regarding timer and I
> thought it would cover things like udelay() as well. I'll add a TODO
> item for now but please let me know if you have something different in
> mind.

Not local to nova-core, but in the generic abstraction. I don't think the
generic abstraction posted on the mailing list contains udelay(). Should be
trivial to add it with a subsequent patch though.

A TODO should be fine for now.

> >> +    let reg_fuse_version = bar.read32(reg_fuse);
> >
> > I feel like the calculation of reg_fuse should be abstracted with a dedicated
> > type in regs.rs. that takes the magic number derived from the engine_id_mask
> > (which I assume is chip specific) and the ucode_id.
> 
> We would need proper support for register arrays to manage the ucode_id
> offset, so I'm afraid this one will be hard to get rid of. What kind of
> type did you have in mind?
> 
> One thing we can do though, is expose the offset of each register as a
> register type constant, and use that instead of the hardcoded values
> currently in this code - that part at least will be cleaner.

Let's do that then for now.

> >> +        let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
> >
> > Just `_` instead? Also, please add a comment why it is important to create this
> > instance even though it's never used.
> 
> It is not really important now, more a way to exercise the code until
> we need to run Booter. The variable will be renamed to `sec2_falcon`
> eventually, so I'd like to keep that name in the placeholder.

Ok, seems reasonable.