Re: [PATCH v6 11/11] gpu: nova-core: add PIO support for loading firmware images

["Danilo Krummrich" <dakr@kernel.org>]

On Wed Jan 14, 2026 at 8:29 PM CET, Timur Tabi wrote:
> +    /// Write a byte array to Falcon memory using programmed I/O (PIO).
> +    ///
> +    /// Writes `img` to the specified `target_mem` (IMEM or DMEM) starting at `mem_base`.
> +    /// For IMEM writes, tags are set for each 256-byte block starting from `tag`.
> +    ///
> +    /// Returns `EINVAL` if `img.len()` is not a multiple of 4.
> +    fn pio_wr_bytes(
> +        &self,
> +        bar: &Bar0,
> +        img: &[u8],
> +        mem_base: u16,
> +        target_mem: FalconMem,
> +        port: u8,

This looks like we should use the Bounded type instead.

> +        tag: u16,
> +    ) -> Result {
> +        // Rejecting misaligned images here allows us to avoid checking
> +        // inside the loops.
> +        if img.len() % 4 != 0 {
> +            return Err(EINVAL);
> +        }
> +
> +        let port = usize::from(port);
> +
> +        match target_mem {
> +            FalconMem::ImemSecure | FalconMem::ImemNonSecure => {
> +                regs::NV_PFALCON_FALCON_IMEMC::default()
> +                    .set_secure(target_mem == FalconMem::ImemSecure)
> +                    .set_aincw(true)
> +                    .set_offs(mem_base)
> +                    .write(bar, &E::ID, port);
> +
> +                let mut tag = tag;
> +                for block in img.chunks(256) {

	for (n, block) in img.chunks(256).iter().enumerate() {
	    regs::NV_PFALCON_FALCON_IMEMT::default()
	        .set_tag(tag + n)
	        .write(bar, &E::ID, port);
	}

This way you don't need the mutable shadow of tag. Besides that, how do we know
this doesn't overflow? Don't we need a checked_add()?

> +                    regs::NV_PFALCON_FALCON_IMEMT::default()
> +                        .set_tag(tag)
> +                        .write(bar, &E::ID, port);
> +                    for word in block.chunks_exact(4) {
> +                        let w = [word[0], word[1], word[2], word[3]];
> +                        regs::NV_PFALCON_FALCON_IMEMD::default()
> +                            .set_data(u32::from_le_bytes(w))
> +                            .write(bar, &E::ID, port);
> +                    }
> +                    tag += 1;
> +                }
> +            }
> +            FalconMem::Dmem => {
> +                regs::NV_PFALCON_FALCON_DMEMC::default()
> +                    .set_aincw(true)
> +                    .set_offs(mem_base)
> +                    .write(bar, &E::ID, port);
> +
> +                for block in img.chunks(256) {
> +                    for word in block.chunks_exact(4) {
> +                        let w = [word[0], word[1], word[2], word[3]];
> +                        regs::NV_PFALCON_FALCON_DMEMD::default()
> +                            .set_data(u32::from_le_bytes(w))
> +                            .write(bar, &E::ID, port);
> +                    }
> +                }
> +            }
> +        }
> +
> +        Ok(())
> +    }
> +
> +    fn pio_wr<F: FalconFirmware<Target = E>>(
> +        &self,
> +        bar: &Bar0,
> +        fw: &F,
> +        target_mem: FalconMem,
> +        load_offsets: &FalconLoadTarget,
> +        port: u8,
> +        tag: u16,
> +    ) -> Result {
> +        let start = usize::from_safe_cast(load_offsets.src_start);
> +        let len = usize::from_safe_cast(load_offsets.len);
> +        let mem_base = u16::try_from(load_offsets.dst_start)?;
> +
> +        // SAFETY: we are the only user of the firmware image at this stage
> +        let data = unsafe { fw.as_slice(start, len).map_err(|_| EINVAL)? };

Why do we need the firmware image to be backed by a DMA object at this point
when you load the firmware image through PIO anyways?

> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
> index 49501aa6ff7f..3a882b7d8aa8 100644
> --- a/drivers/gpu/nova-core/falcon/hal.rs
> +++ b/drivers/gpu/nova-core/falcon/hal.rs
> @@ -15,6 +15,11 @@
>  mod ga102;
>  mod tu102;
>  
> +pub(crate) enum LoadMethod {
> +    Pio,
> +    Dma,
> +}

Seems obvious, but still, please add some documentation explaining that this is
the load method for falcon firmware images, etc.

> +pub(crate) struct GenericBootloader {
> +    pub desc: BootloaderDesc,
> +    pub ucode: Vec<u8, kernel::alloc::allocator::Kmalloc>,

	pub ucode: KVec<u8>,

Also, we may want to use KVVec<u8> or just VVec<u8> instead. What's the image
size?

> +}
> +
>  /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP falcon.
>  ///
>  /// It is responsible for e.g. carving out the WPR2 region as the first step of the GSP bootflow.
> @@ -221,6 +286,8 @@ pub(crate) struct FwsecFirmware {
>      desc: FalconUCodeDesc,
>      /// GPU-accessible DMA object containing the firmware.
>      ucode: FirmwareDmaObject<Self, Signed>,
> +    /// Generic bootloader
> +    gen_bootloader: Option<GenericBootloader>,

I'm not convinced this is a good idea. We probably want a HAL here and have
different FwsecFirmware types:

One with a DMA object and one with a system memory object when the architecture
uses PIO. In the latter case the object can have a GenericBootloader field, i.e.
this also gets us rid of the Option and all the subsequent 'if chipset <
Chipset::GA102' checks and 'match gbl_fw' matches below.

>  }
>  
>  impl FalconLoadParams for FwsecFirmware {
> @@ -275,7 +342,19 @@ fn brom_params(&self) -> FalconBromParams {
>      }
>  
>      fn boot_addr(&self) -> u32 {
> -        0
> +        match &self.desc {
> +            FalconUCodeDesc::V2(_v2) => {
> +                // 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.
> +                if let Some(ref gbl) = self.gen_bootloader {
> +                    gbl.desc.start_tag << 8
> +                } else {
> +                    0
> +                }
> +            }
> +            FalconUCodeDesc::V3(_v3) => 0,
> +        }
>      }
>  }
>  
> @@ -376,6 +455,7 @@ impl FwsecFirmware {
>      /// command.
>      pub(crate) fn new(
>          dev: &Device<device::Bound>,
> +        chipset: Chipset,
>          falcon: &Falcon<Gsp>,
>          bar: &Bar0,
>          bios: &Vbios,
> @@ -432,9 +512,54 @@ pub(crate) fn new(
>              ucode_dma.no_patch_signature()
>          };
>  
> +        // The Generic Bootloader exists only on Turing and GA100.  To avoid a bogus
> +        // console error message on other platforms, only try to load it if it's
> +        // supposed to be there.
> +        let gbl_fw = if chipset < Chipset::GA102 {
> +            Some(super::request_firmware(
> +                dev,
> +                chipset,
> +                "gen_bootloader",
> +                FIRMWARE_VERSION,
> +            )?)
> +        } else {
> +            None
> +        };
> +
> +        let gbl = match gbl_fw {
> +            Some(fw) => {
> +                let hdr = fw
> +                    .data()
> +                    .get(0..size_of::<BinHdr>())
> +                    .and_then(BinHdr::from_bytes_copy)
> +                    .ok_or(EINVAL)?;
> +
> +                let desc_offset = usize::from_safe_cast(hdr.header_offset);
> +                let desc = fw
> +                    .data()
> +                    .get(desc_offset..desc_offset + size_of::<BootloaderDesc>())
> +                    .and_then(BootloaderDesc::from_bytes_copy)
> +                    .ok_or(EINVAL)?;
> +
> +                let ucode_start = usize::from_safe_cast(hdr.data_offset);
> +                let ucode_size = usize::from_safe_cast(hdr.data_size);
> +                let ucode_data = fw
> +                    .data()
> +                    .get(ucode_start..ucode_start + ucode_size)
> +                    .ok_or(EINVAL)?;
> +
> +                let mut ucode = KVec::new();
> +                ucode.extend_from_slice(ucode_data, GFP_KERNEL)?;
> +
> +                Some(GenericBootloader { desc, ucode })
> +            }
> +            None => None,
> +        };
> +
>          Ok(FwsecFirmware {
>              desc,
>              ucode: ucode_signed,
> +            gen_bootloader: gbl,
>          })
>      }