Message-ID: <dcd18bd8-cb7e-4c28-b4c6-95a201710270@nvidia.com>
Date: Fri, 5 Dec 2025 12:15:29 -0500
From: Joel Fernandes <joelagnelf@...dia.com>
To: John Hubbard <jhubbard@...dia.com>, Danilo Krummrich <dakr@...nel.org>
Cc: Alexandre Courbot <acourbot@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 Alistair Popple <apopple@...dia.com>, Edwin Peer <epeer@...dia.com>,
 Zhi Wang <zhiw@...dia.com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Bjorn Helgaas <bhelgaas@...gle.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 <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 nouveau@...ts.freedesktop.org, rust-for-linux@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 23/31] gpu: nova-core: Hopper/Blackwell: add FSP Chain of
 Trust boot

Hi John,

On 12/3/2025 12:59 AM, John Hubbard wrote:
> +    ///
> +    /// This version takes pre-extracted signatures and FMC image data.
> +    /// Used when signatures are extracted separately from the full ELF file.
> +    #[allow(clippy::too_many_arguments)]
> +    pub(crate) fn boot_gsp_fmc_with_signatures(
> +        dev: &device::Device<device::Bound>,
> +        bar: &crate::driver::Bar0,
> +        chipset: crate::gpu::Chipset,
> +        fmc_image_fw: &crate::dma::DmaObject, // Contains only the image section
> +        fmc_boot_params: &kernel::dma::CoherentAllocation<GspFmcBootParams>,
> +        total_reserved_size: u64,
> +        resume: bool,
> +        fsp_falcon: &crate::falcon::Falcon<crate::falcon::fsp::Fsp>,
> +        signatures: &FmcSignatures,
> +    ) -> Result<()> {
> +        dev_dbg!(dev, "Starting FSP boot sequence for {}\n", chipset);

I see lots of dev_dbg left as you also pointed. I guess you will remove these as
we discussed in the other thread.

> +
> +        // Build FSP Chain of Trust message
> +        let fmc_addr = fmc_image_fw.dma_handle(); // Now points to image data only
> +        let fmc_boot_params_addr = fmc_boot_params.dma_handle();
> +
> +        // frts_offset is relative to FB end: FRTS_location = FB_END - frts_offset
> +        let frts_offset = if !resume {
> +            let mut frts_reserved_size = if chipset.needs_large_reserved_mem() {
> +                0x220000 // heap_size_non_wpr for Hopper/Blackwell+

Can we please use constants for the magic numbers? Also with comment headers
clearly documenting the constants where they are defined.

> +            } else {
> +                total_reserved_size
> +            };
> +
> +            // Add PMU reserved size
> +            frts_reserved_size += u64::from(crate::fb::PMU_RESERVED_SIZE);
> +
> +            frts_reserved_size
> +                .align_up(Alignment::new::<0x200000>())
> +                .unwrap_or(frts_reserved_size)
> +        } else {
> +            0
> +        };
> +        let frts_size = if !resume { 0x100000 } else { 0 }; // 1MB FRTS size


Should use SZ_ constants, here and everywhere. Like SZ_1M etc [1].

[1] https://rust.docs.kernel.org/src/kernel/sizes.rs.html

> +
> +        // Build the FSP message
> +        let msg = KBox::new(
> +            FspMessage {
> +                mctp_header: (mctp::HEADER_SOM << 31)
> +                    | (mctp::HEADER_EOM << 30)
> +                    | (mctp::HEADER_SEID << 16)
> +                    | (mctp::HEADER_SEQ << 28),
> +
> +                nvdm_header: (mctp::MSG_TYPE_VENDOR_PCI)
> +                    | (mctp::VENDOR_ID_NV << 8)
> +                    | (mctp::NVDM_TYPE_COT << 24),
> +
> +                cot: NvdmPayloadCot {
> +                    version: FSP_COT_VERSION,
> +                    size: core::mem::size_of::<NvdmPayloadCot>() as u16,
> +                    gsp_fmc_sysmem_offset: fmc_addr,
> +                    frts_sysmem_offset: 0,
> +                    frts_sysmem_size: 0,
> +                    frts_vidmem_offset: frts_offset,
> +                    frts_vidmem_size: frts_size,
> +                    hash384: signatures.hash384,
> +                    public_key: signatures.public_key,
> +                    signature: signatures.signature,
> +                    gsp_boot_args_sysmem_offset: fmc_boot_params_addr,
> +                },
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        // Convert message to bytes for sending
> +        let msg_bytes = msg.as_bytes();
> +
> +        dev_dbg!(
> +            dev,
> +            "FSP COT Message:\n  size={} bytes\n  fmc_addr={:#x}\n  boot_params={:#x}\n  \
> +             frts_offset={:#x}\n  frts_size={:#x}\n",
> +            msg_bytes.len(),
> +            fmc_addr,
> +            fmc_boot_params_addr,
> +            frts_offset,
> +            frts_size
> +        );
> +
> +        // Send COT message to FSP and wait for response
> +        Self::send_sync_fsp(dev, bar, fsp_falcon, mctp::NVDM_TYPE_COT, msg_bytes)?;

This seems to diverge from the pattern we use in GSP `cmdq`. Can we keep it
consistent, like `send_sync_fsp<M>` ?

    pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
    where
        M: CommandToGsp,

Alistair/Alex may have more design suggestions since they came up with this
pattern. Especially around safety (guarantees that the part of buffer being read
will not be modified by HW etc).

Over all, I feel we have 3 different mechanisms now (1 upstream, 2 in the
works). The GSP cmdq, RM control and then this one. It would be good to get some
consistency in the API designs for all these different mechanisms (and possibly
share any common code).

Thanks!

 - Joel

Powered by blists - more mailing lists