Re: [PATCH v10 08/10] gpu: nova-core: use the Generic Bootloader to boot FWSEC on Turing

["Eliot Courtney" <ecourtney@nvidia.com>]

On Sun Mar 1, 2026 at 11:03 PM JST, Alexandre Courbot wrote:
> From: Timur Tabi <ttabi@nvidia.com>
> +impl FwsecFirmwareWithBl {
> +    /// Loads the bootloader firmware for `dev` and `chipset`, and wrap `firmware` so it can be
> +    /// loaded using it.
> +    pub(crate) fn new(
> +        firmware: FwsecFirmware,
> +        dev: &Device<device::Bound>,
> +        chipset: Chipset,
> +    ) -> Result<Self> {
> +        let fw = request_firmware(dev, chipset, "gen_bootloader", FIRMWARE_VERSION)?;
> +        let hdr = fw
> +            .data()
> +            .get(0..size_of::<BinHdr>())
> +            .and_then(BinHdr::from_bytes_copy)
> +            .ok_or(EINVAL)?;
> +
> +        let desc = {
> +            let desc_offset = usize::from_safe_cast(hdr.header_offset);
> +
> +            fw.data()
> +                .get(desc_offset..)
> +                .and_then(BootloaderDesc::from_bytes_copy_prefix)
> +                .ok_or(EINVAL)?
> +                .0
> +        };
> +
> +        let ucode = {
> +            let ucode_start = usize::from_safe_cast(hdr.data_offset);
> +            let code_size = usize::from_safe_cast(desc.code_size);
> +            // Align to falcon block size (256 bytes).
> +            let aligned_code_size = code_size
> +                .align_up(Alignment::new::<{ falcon::MEM_BLOCK_ALIGNMENT }>())
> +                .ok_or(EINVAL)?;
> +
> +            let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
> +            ucode.extend_from_slice(
> +                fw.data()
> +                    .get(ucode_start..ucode_start + code_size)
> +                    .ok_or(EINVAL)?,
> +                GFP_KERNEL,
> +            )?;
> +            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
> +
> +            ucode
> +        };
> +
> +        let firmware_dma = DmaObject::from_data(dev, &firmware.ucode.0)?;
> +
> +        let dmem_desc = {
> +            let imem_sec = FalconDmaLoadable::imem_sec_load_params(&firmware);
> +            let imem_ns = FalconDmaLoadable::imem_ns_load_params(&firmware).ok_or(EINVAL)?;
> +            let dmem = FalconDmaLoadable::dmem_load_params(&firmware);
> +
> +            BootloaderDmemDescV2 {
> +                reserved: [0; 4],
> +                signature: [0; 4],
> +                ctx_dma: 4, // FALCON_DMAIDX_PHYS_SYS_NCOH
> +                code_dma_base: firmware_dma.dma_handle(),
> +                non_sec_code_off: imem_ns.dst_start,
> +                non_sec_code_size: imem_ns.len,
> +                sec_code_off: imem_sec.dst_start,

Is it correct here to use `dst_start` for `non_sec_code_off` and `sec_code_off`?
AFAICT, these are meant to be offsets from the base of the DMA memory and
it's meant to copy into the falcon. But the documentation on `dst_start`
says `Offset from the start of the destination memory to copy into.`

Also `ucode_start` is defined as `hdr.data_offset`
but doesn't add `code_off` from `BootloaderDesc` and `code_off` appears
unused. So does `data_off` and `dmem_load_off` for that matter.
I find it hard to follow what's actually required but it seems odd that
none of these are used.

At least for `code_off` should it not be part of the computation of `ucode_start`?
`let ucode_start = usize::from_safe_cast(hdr.data_offset);`

Overall I find the dst/srcs here confusing cos it feels hard to keep
track of which set of memory things are in. So, sorry if these comments
are nonsense.

> +
> +        Ok(Self {
> +            _firmware_dma: firmware_dma,
> +            ucode,
> +            dmem_desc,
> +            brom_params: firmware.brom_params(),
> +            imem_dst_start: u16::try_from(imem_dst_start)?,
> +            start_tag: u16::try_from(desc.start_tag)?,
> +        })
> +    }
> +
> +    /// Loads the bootloader into `falcon` and execute it.
> +    ///
> +    /// The bootloader will load the FWSEC firmware and then execute it. This function returns
> +    /// after FWSEC has reached completion.
> +    pub(crate) fn run(
> +        &self,
> +        dev: &Device<device::Bound>,
> +        falcon: &Falcon<Gsp>,
> +        bar: &Bar0,
> +    ) -> Result<()> {
> +        // Reset falcon, load the firmware, and run it.
> +        falcon
> +            .reset(bar)
> +            .inspect_err(|e| dev_err!(dev, "Failed to reset GSP falcon: {:?}\n", e))?;
> +        falcon
> +            .pio_load(bar, self)
> +            .inspect_err(|e| dev_err!(dev, "Failed to load FWSEC firmware: {:?}\n", e))?;
> +
> +        // Configure DMA index for the bootloader to fetch the FWSEC firmware from system memory.
> +        regs::NV_PFALCON_FBIF_TRANSCFG::try_update(
> +            bar,
> +            &Gsp::ID,
> +            usize::from_safe_cast(self.dmem_desc.ctx_dma),
> +            |v| {
> +                v.set_target(FalconFbifTarget::CoherentSysmem)
> +                    .set_mem_type(FalconFbifMemType::Physical)
> +            },
> +        )?;
> +
> +        let (mbox0, _) = falcon
> +            .boot(bar, Some(0), None)
> +            .inspect_err(|e| dev_err!(dev, "Failed to boot FWSEC firmware: {:?}\n", e))?;
> +        if mbox0 != 0 {
> +            dev_err!(dev, "FWSEC firmware returned error {}\n", mbox0);
> +            Err(EIO)
> +        } else {
> +            Ok(())
> +        }
> +    }
> +}
> +
> +impl FalconFirmware for FwsecFirmwareWithBl {
> +    type Target = Gsp;
> +
> +    fn brom_params(&self) -> FalconBromParams {
> +        self.brom_params.clone()
> +    }
> +
> +    fn boot_addr(&self) -> u32 {
> +        // On V2 platforms, the boot address is extracted from the generic bootloader, because the
> +        // gbl is what actually copies FWSEC into memory, so that is what needs to be booted.
> +        u32::from(self.start_tag) << 8
> +    }
> +}
> +
> +impl FalconPioLoadable for FwsecFirmwareWithBl {
> +    fn imem_sec_load_params(&self) -> Option<FalconPioImemLoadTarget<'_>> {
> +        None
> +    }
> +
> +    fn imem_ns_load_params(&self) -> Option<FalconPioImemLoadTarget<'_>> {
> +        Some(FalconPioImemLoadTarget {
> +            data: self.ucode.as_ref(),
> +            dst_start: self.imem_dst_start,
> +            secure: false,
> +            start_tag: self.start_tag,
> +        })
> +    }
> +
> +    fn dmem_load_params(&self) -> FalconPioDmemLoadTarget<'_> {
> +        FalconPioDmemLoadTarget {
> +            data: self.dmem_desc.as_bytes(),
> +            dst_start: 0,
> +        }
> +    }
> +}

Here in `FwsecFirmwareWithBl::dmem_load_params` it returns dst_start as 0,
except the docs say 'This has to be filled by the GPU driver and copied into DMEM at offset
/// [`BootloaderDesc.dmem_load_off`].'


> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index 78957ed8814f..9a00ddb922ac 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -24,6 +24,7 @@
>              BooterKind, //
>          },
>          fwsec::{
> +            bootloader::FwsecFirmwareWithBl,
>              FwsecCommand,
>              FwsecFirmware, //
>          },
> @@ -48,6 +49,7 @@ impl super::Gsp {
>      /// created the WPR2 region.
>      fn run_fwsec_frts(
>          dev: &device::Device<device::Bound>,
> +        chipset: Chipset,
>          falcon: &Falcon<Gsp>,
>          bar: &Bar0,
>          bios: &Vbios,
> @@ -63,6 +65,7 @@ fn run_fwsec_frts(
>              return Err(EBUSY);
>          }
>  
> +        // FWSEC-FRTS will create the WPR2 region.
>          let fwsec_frts = FwsecFirmware::new(
>              dev,
>              falcon,
> @@ -74,8 +77,14 @@ fn run_fwsec_frts(
>              },
>          )?;
>  
> -        // Run FWSEC-FRTS to create the WPR2 region.
> -        fwsec_frts.run(dev, falcon, bar)?;
> +        if chipset.needs_fwsec_bootloader() {
> +            let fwsec_frts_bl = FwsecFirmwareWithBl::new(fwsec_frts, dev, chipset)?;
> +            // Load and run the bootloader, which will load FWSEC-FRTS and run it.
> +            fwsec_frts_bl.run(dev, falcon, bar)?;
> +        } else {
> +            // Load and run FWSEC-FRTS directly.
> +            fwsec_frts.run(dev, falcon, bar)?;
> +        }
>  
>          // SCRATCH_E contains the error code for FWSEC-FRTS.
>          let frts_status = regs::NV_PBUS_SW_SCRATCH_0E_FRTS_ERR::read(bar).frts_err_code();
> @@ -144,7 +153,7 @@ pub(crate) fn boot(
>          let fb_layout = FbLayout::new(chipset, bar, &gsp_fw)?;
>          dev_dbg!(dev, "{:#x?}\n", fb_layout);
>  
> -        Self::run_fwsec_frts(dev, gsp_falcon, bar, &bios, &fb_layout)?;
> +        Self::run_fwsec_frts(dev, chipset, gsp_falcon, bar, &bios, &fb_layout)?;
>  
>          let booter_loader = BooterFirmware::new(
>              dev,