Re: [PATCH 23/31] gpu: nova-core: Hopper/Blackwell: add FSP Chain of Trust boot

[John Hubbard <jhubbard@nvidia.com>]

On 12/5/25 9:15 AM, Joel Fernandes wrote:
> 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.

Yes. I am thinking of leaving in a very few items (like 1 or 2), as sort of
progress indicators, but even those are up for veto of course.

> 
>> +
>> +        // 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].

Yes, I'm still glitching and not always doing that, good catch.

> 
> [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).
> 

I'll try to catch up with the GSP cmdq approach and apply it.

thanks,
-- 
John Hubbard