On Sun Mar 1, 2026 at 11:03 PM JST, Alexandre Courbot wrote:
> From: Timur Tabi <[email protected]>
> +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,
