Re: [PATCH v3 13/33] gpu: nova-core: separate the generic falcon bootloader from FWSEC

[Alexandre Courbot]

On Fri Sep 18, 2026 at 10:06 AM JST, John Hubbard wrote:
> The generic falcon bootloader is a small program that loads a larger
> image into a falcon. The r000 boot protocol needs it to run an image
> that GSP-RM names in a load-and-execute event during boot.
>
> Nova-core used the bootloader only to load FWSEC, so the bootloader code
> was part of the FWSEC code, and there was no way to run it on another
> image.
>
> Move the bootloader into its own type, which FWSEC then uses.
>
> Place the bootloader at the top of the falcon's IMEM, using the IMEM
> size that the HWCFG register reports, instead of assuming a 64 KiB IMEM.
>
> Assisted-by: LLM
> Reviewed-by: Timur Tabi <ttabi@xxxxxxxxxx>
> Reviewed-by: Zhi Wang <zhiw@xxxxxxxxxx>
> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>

This is a very good change to have even without the switch to r000 as it
properly isolates the generic bootloader code. But this change alone is
not the entire story: patch 16 is another part of it, and so are the
bits of patch 17 that touch `fwsec/bootloader.rs` and
`gen_bootloader.rs`. If you extract the relevant bits of patch 17 into a
new patch then you have 3 commits that read perfectly once they are
put in a sequence (i.e. the current patches 14 and 15 should be moved
right after the extracted part of patch 17).

I've tried squashing these 3 patches into a single one and even that
looks pretty decent with `--color-moved`, but it's also fine to keep
3 separate patches if you prefer.

Those patches also still seem to have some doccomment churn, please
confirm whether changing the documentation is actually warranted.

Just one tiny nit on this patch below.

<...>
> +impl GenericBootloader {
> +    /// Loads the generic bootloader image for `chipset`, placed in the last blocks of an IMEM of
> +    /// `imem_size` bytes.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `EINVAL` if a required TLV field is absent or the image does not fit in IMEM.
> +    /// - `ENOMEM` if the padded copy of the code cannot be allocated.
> +    pub(crate) fn new(
> +        dev: &device::Device<device::Bound>,
> +        chipset: Chipset,
> +        imem_size: usize,
> +    ) -> Result<Self> {
> +        let fw = request_tlv(dev, chipset, "gen_bootloader")?;
> +        let tlv = Tlv::new(fw.data())?;
> +        dev_dbg!(
> +            dev,
> +            "loaded generic bootloader firmware v{}\n",
> +            tlv.get_string(b"VERS")?
> +        );
> +
> +        let ucode = {
> +            let blob = tlv.get_bytes(b"BLOB")?;
> +            let code_size = usize::from_safe_cast(tlv.get_u32(b"CDSZ")?);
> +            let code = blob.get(..code_size).ok_or(EINVAL)?;
> +            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(code, GFP_KERNEL)?;
> +            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
> +
> +            ucode
> +        };
> +
> +        // The top of IMEM, above the blocks that the bootloader loads the image into.
> +        let imem_dst_start = imem_size.checked_sub(ucode.len()).ok_or(EINVAL)?;
> +
> +        Ok(Self {
> +            ucode,
> +            imem_dst_start: u16::try_from(imem_dst_start)?,
> +            start_tag: u16::try_from(tlv.get_u32(b"STRT")?)?,
> +        })
> +    }
> +
> +    pub(crate) fn boot_addr(&self) -> u32 {

Let's give a short documentation to this new method.