Re: [PATCH v4 20/33] gpu: nova-core: Hopper/Blackwell: add FSP secure boot completion waiting

[John Hubbard]

On 2/17/26 9:13 AM, Danilo Krummrich wrote:
> On Tue Feb 10, 2026 at 3:45 AM CET, John Hubbard wrote:
>> +/// MCTP (Management Component Transport Protocol) header values for FSP communication.
>> +pub(crate) mod mctp {
>> +    pub(super) const HEADER_SOM: u32 = 1; // Start of Message
>> +    pub(super) const HEADER_EOM: u32 = 1; // End of Message
>> +    pub(super) const HEADER_SEID: u32 = 0; // Source Endpoint ID
>> +    pub(super) const HEADER_SEQ: u32 = 0; // Sequence number
> 
> This looks like it should be a MctpMessageType enum instead.

Done. Or it's moral equivalent, anyway: I've overhauled this whole
thing, put it in its own module and file and patch, and used types
throughout.

> 
>> +
>> +    pub(super) const MSG_TYPE_VENDOR_PCI: u32 = 0x7e;
>> +    pub(super) const VENDOR_ID_NV: u32 = 0x10de;
>> +    pub(super) const NVDM_TYPE_COT: u32 = 0x14;
>> +    pub(super) const NVDM_TYPE_FSP_RESPONSE: u32 = 0x15;
> 
> This seems like it should be a different type (or even types) as they are
> specific header field values.

I think you'll like the new arrangement in v5.

> 
>> +}
>> +
>> +/// GSP FMC boot parameters structure.
>> +/// This is what FSP expects to receive for booting GSP-RM.
>> +/// GSP FMC initialization parameters.
>> +#[repr(C)]
>> +#[derive(Debug, Clone, Copy, Default)]
>> +struct GspFmcInitParams {
>> +    /// CC initialization "registry keys"
>> +    regkeys: u32,
>> +}
>> +
>> +// SAFETY: GspFmcInitParams is a simple C struct with only primitive types.
>> +unsafe impl AsBytes for GspFmcInitParams {}
>> +// SAFETY: All bit patterns are valid for the primitive fields.
>> +unsafe impl FromBytes for GspFmcInitParams {}
>> +
>> +/// GSP ACR (Authenticated Code RAM) boot parameters.
>> +#[repr(C)]
>> +#[derive(Debug, Clone, Copy, Default)]
>> +struct GspAcrBootGspRmParams {
>> +    /// Physical memory aperture through which gspRmDescPa is accessed
> 
> For the whole series, please end them with a period. Personally, I don't care
> too much, but it's a convention we have for Rust code.

Fixed.

> 
>> +/// FSP interface for Hopper/Blackwell GPUs.
>> +pub(crate) struct Fsp;
>> +
>> +impl Fsp {
>> +    /// Wait for FSP secure boot completion.
>> +    ///
>> +    /// Polls the thermal scratch register until FSP signals boot completion
>> +    /// or timeout occurs.
>> +    pub(crate) fn wait_secure_boot(
>> +        dev: &device::Device<device::Bound>,
>> +        bar: &crate::driver::Bar0,
>> +        arch: crate::gpu::Architecture,
>> +    ) -> Result<()> {
> 
> Please just use 'Result'.

Fixed everywhere. (And more than once, since it came back a couple
of times during a rebase glitch just now, haha.)

> 
>> +        let timeout = Delta::from_millis(FSP_SECURE_BOOT_TIMEOUT_MS);
>> +
>> +        read_poll_timeout(
>> +            || crate::regs::read_fsp_boot_complete_status(bar, arch),
> 
> Let's just import regs.

Done.

> 
>> +            |&status| {
>> +                dev_dbg!(
>> +                    dev,
>> +                    "FSP I2CS scratch register status: {:#x} (expected: {:#x})\n",
>> +                    status,
>> +                    FSP_BOOT_COMPLETE_SUCCESS
>> +                );
> 
> I don't think we should have print statements in the condition closure. Even for
> debug prints we don't want to spam the console.

Done.

> 
>> +                status == FSP_BOOT_COMPLETE_SUCCESS
>> +            },
>> +            Delta::ZERO,
>> +            timeout,
>> +        )
>> +        .map_err(|_| {
>> +            let final_status =
>> +                crate::regs::read_fsp_boot_complete_status(bar, arch).unwrap_or(0xDEADBEEF);
> 
> I think read_fsp_boot_complete_status() should just return an Option.

Done.

> 
> Also, do we really care about the actual value if we time out? If so, this won't
> work reliably, i.e. final_status could have changed to
> FSP_BOOT_COMPLETE_SUCCESS.
> 
>> +            dev_err!(
>> +                dev,
>> +                "FSP secure boot completion timeout - final status: {:#x}\n",
>> +                final_status
>> +            );
>> +            ETIMEDOUT
> 
> Even if it is the wrong architecture we return ETIMEDOUT? Actually, since FSP is
> for Hopper/Blackwell only/plus, we way even want to print a warning and use
> debug_assert().

Right, I added a debug_assert(), and removed the re-read in the error case.

> 
>> +        })
>> +        .map(|_| ())
>> +    }
>> +}

thanks,
-- 
John Hubbard