linux-kernel - Re: [PATCH v4 14/20] 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: <DACQP9IFQ89O.3HILG5TOLU8H6@nvidia.com>
Date: Tue, 03 Jun 2025 17:03:02 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Lyude Paul" <lyude@...hat.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" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Danilo
 Krummrich" <dakr@...nel.org>, "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>
Cc: "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 v4 14/20] gpu: nova-core: add falcon register
 definitions and base code

Hi Lyude, thanks for the review!

On Sat May 31, 2025 at 7:22 AM JST, Lyude Paul wrote:
<snip>
>> +    /// `target_mem`.
>> +    ///
>> +    /// `sec` is set if the loaded firmware is expected to run in secure mode.
>> +    fn dma_wr(
>> +        &self,
>> +        bar: &Bar0,
>> +        dma_handle: bindings::dma_addr_t,
>> +        target_mem: FalconMem,
>> +        load_offsets: FalconLoadTarget,
>> +        sec: bool,
>> +    ) -> Result {
>> +        const DMA_LEN: u32 = 256;
>> +
>> +        // For IMEM, we want to use the start offset as a virtual address tag for each page, since
>> +        // code addresses in the firmware (and the boot vector) are virtual.
>> +        //
>> +        // For DMEM we can fold the start offset into the DMA handle.
>> +        let (src_start, dma_start) = match target_mem {
>> +            FalconMem::Imem => (load_offsets.src_start, dma_handle),
>> +            FalconMem::Dmem => (
>> +                0,
>> +                dma_handle + load_offsets.src_start as bindings::dma_addr_t,
>
> I wonder if maybe we shouldn't use dma_addr_t directly from bindings and add a
> proper type alias for it somewhere?

I guess so, let me see if I can easily change CoherentAllocation to
support that (and address Danilo's related comment).

>> +pub(crate) trait FalconHal<E: FalconEngine>: Sync {
>> +    // Activates the Falcon core if the engine is a risvc/falcon dual engine.
>> +    fn select_core(&self, _falcon: &Falcon<E>, _bar: &Bar0) -> Result<()> {
>
> Could just be Result
>
>> +        Ok(())
>> +    }
>> +
>> +    /// Returns the fused version of the signature to use in order to run a HS firmware on this
>> +    /// falcon instance. `engine_id_mask` and `ucode_id` are obtained from the firmware header.
>> +    fn get_signature_reg_fuse_version(
>> +        &self,
>> +        falcon: &Falcon<E>,
>> +        bar: &Bar0,
>> +        engine_id_mask: u16,
>> +        ucode_id: u8,
>> +    ) -> Result<u32>;
>> +
>> +    // Program the boot ROM registers prior to starting a secure firmware.
>> +    fn program_brom(&self, falcon: &Falcon<E>, bar: &Bar0, params: &FalconBromParams)
>> +        -> Result<()>;
>
> Same here

Urrk I've done this throughout the code. Fixed them all, thanks.