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

Danilo Krummrich
2026-02-17T17:13:47Z

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.

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

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

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

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

> + |&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.

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

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

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