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

John Hubbard
2026-02-20T23:26:51Z

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