Re: [PATCH v3 3/7] gpu: nova-core: transition booter_load to TLV images

On Fri Jul 3, 2026 at 4:27 AM JST, Timur Tabi wrote:
<...>
>  /// Signature for Booter firmware. Their size is encoded into the header and not known a compile
>  /// time, so we just wrap a byte slices on which we can implement [`FirmwareSignature`].
>  struct BooterSignature<'a>(&'a [u8]);
> @@ -291,85 +85,91 @@ pub(crate) fn new(
>          dev: &device::Device<device::Bound>,
>          kind: BooterKind,
>          chipset: Chipset,
> -        ver: &str,
>          falcon: &Falcon<'_, <Self as FalconFirmware>::Target>,
>      ) -> Result<Self> {
>          let fw_name = match kind {
>              BooterKind::Loader => "booter_load",
>              BooterKind::Unloader => "booter_unload",
>          };
> -        let fw = super::request_firmware(dev, chipset, fw_name, ver)?;
> -        let bin_fw = BinFirmware::new(&fw)?;
> -
> -        // The binary firmware embeds a Heavy-Secured firmware.
> -        let hs_fw = HsFirmwareV2::new(&bin_fw)?;
> -
> -        // The Heavy-Secured firmware embeds a firmware load descriptor.
> -        let load_hdr = HsLoadHeaderV2::new(&hs_fw)?;
> -
> -        // Offset in `ucode` where to patch the signature.
> -        let patch_loc = hs_fw.patch_location()?;
> +        let fw = request_tlv(dev, chipset, fw_name)?;
> +        let tlv = Tlv::new(fw.data())?;
> +        dev_dbg!(
> +            dev,
> +            "loaded {} firmware v{}\n",
> +            fw_name,
> +            tlv.get_string(b"VERS")?
> +        );
> +
> +        let os_data_offset = tlv.get_u32(b"DAOF")?;
> +        let os_data_size = tlv.get_u32(b"DASZ")?;
> +        let os_code_offset = tlv.get_u32(b"CDOF")?;
> +        let os_code_size = tlv.get_u32(b"CDSZ")?;
> +        let patch_loc = tlv.get_u32(b"PLOC")?;
> +        let fuse_version = tlv.get_u32(b"FUSE")?;
> +        let engine_id = tlv.get_u32(b"ENID")?;
> +        let ucode_id = tlv.get_u32(b"UCID")?;
> +        let app0_code_offset = tlv.get_u32(b"A0CO")?;
> +        let app0_code_size = tlv.get_u32(b"A0CS")?;
> +        let num_sigs = tlv.get_u32(b"NSIG")?;
> +        let sig_bytes = tlv.get_bytes(b"SIGN")?;
> +
> +        // Booter is always signed

The `.rst` file mentions the non-signed case though - we should
reconcile the spec and the code one way or the other.

> +        if !(1..=15).contains(&num_sigs) || sig_bytes.len() % num_sigs as usize != 0 {

s/as usize/into_safe_cast.

> +            dev_err!(dev, "invalid signature count {}\n", num_sigs);
> +            return Err(EINVAL);
> +        }
>  
> -        let sig_params = HsSignatureParams::new(&hs_fw)?;
>          let brom_params = FalconBromParams {
> -            // `load_hdr.os_data_offset` is an absolute index, but `pkc_data_offset` is from the
> +            // `os_data_offset` is an absolute index, but `pkc_data_offset` is from the
>              // signature patch location.
> -            pkc_data_offset: patch_loc
> -                .checked_sub(load_hdr.os_data_offset)
> -                .ok_or(EINVAL)?,
> -            engine_id_mask: u16::try_from(sig_params.engine_id_mask).map_err(|_| EINVAL)?,
> -            ucode_id: u8::try_from(sig_params.ucode_id).map_err(|_| EINVAL)?,
> +            pkc_data_offset: patch_loc.checked_sub(os_data_offset).ok_or(EINVAL)?,
> +            engine_id_mask: u16::try_from(engine_id).map_err(|_| EINVAL)?,
> +            ucode_id: u8::try_from(ucode_id).map_err(|_| EINVAL)?,
>          };
> -        let app0 = HsLoadHeaderV2App::new(&hs_fw, 0)?;
>  
> -        // Object containing the firmware microcode to be signature-patched.
> -        let ucode = bin_fw
> -            .data()
> -            .ok_or(EINVAL)
> +        let ucode = tlv
> +            .get_bytes(b"BLOB")
>              .and_then(FirmwareObject::<Self, _>::new_booter)?;
>  
> -        let ucode_signed = {
> -            let mut signatures = hs_fw.signatures_iter()?.peekable();
> -
> -            if signatures.peek().is_none() {
> -                // If there are no signatures, then the firmware is unsigned.
> -                ucode.no_patch_signature()
> -            } else {
> -                // Obtain the version from the fuse register, and extract the corresponding
> -                // signature.
> -                let reg_fuse_version = falcon
> -                    .signature_reg_fuse_version(brom_params.engine_id_mask, brom_params.ucode_id)?;
> -
> -                // `0` means the last signature should be used.
> -                const FUSE_VERSION_USE_LAST_SIG: u32 = 0;
> -                let signature = match reg_fuse_version {
> -                    FUSE_VERSION_USE_LAST_SIG => signatures.last(),
> -                    // Otherwise hardware fuse version needs to be subtracted to obtain the index.
> -                    reg_fuse_version => {
> -                        let Some(idx) = sig_params.fuse_ver.checked_sub(reg_fuse_version) else {
> -                            dev_err!(dev, "invalid fuse version for Booter firmware\n");
> -                            return Err(EINVAL);
> -                        };
> -                        signatures.nth(idx.into_safe_cast())
> -                    }
> -                }
> -                .ok_or(EINVAL)?;
> -
> -                ucode.patch_signature(&signature, patch_loc.into_safe_cast())?
> -            }
> +        // Obtain the version from the fuse register, and extract the corresponding
> +        // signature.
> +        let reg_fuse_version =
> +            falcon.signature_reg_fuse_version(brom_params.engine_id_mask, brom_params.ucode_id)?;
> +
> +        const FUSE_VERSION_USE_LAST_SIG: u32 = 0;
> +        let index = match reg_fuse_version {
> +            // `0` means the last signature should be used.
> +            FUSE_VERSION_USE_LAST_SIG => num_sigs - 1,
> +            // Otherwise, hardware fuse version needs to be subtracted to obtain the index.
> +            _ => fuse_version.checked_sub(reg_fuse_version).ok_or(EINVAL)?,
>          };
>  
> +        // The size of one signature
> +        let sig_size = sig_bytes
> +            .len()
> +            .checked_div(num_sigs.into_safe_cast())
> +            .ok_or(EINVAL)?;
> +
> +        // Extract the nth signature
> +        let sig_chunk = sig_bytes
> +            .chunks_exact(sig_size)

As Sashiko pointed out, this will panic if `sig_size == 0`, so we need
to check that - and whether an unsigned Booter is valid at all.

> +            .nth(index as usize)

s/as_usize/into_safe_cast.

> +            .ok_or(EINVAL)?;
> +
> +        let signature = BooterSignature(sig_chunk);
> +        let ucode_signed = ucode.patch_signature(&signature, patch_loc.into_safe_cast())?;
> +
>          // There are two versions of Booter, one for Turing/GA100, and another for
>          // GA102+.  The extraction of the IMEM sections differs between the two
>          // versions.  Unfortunately, the file names are the same, and the headers
>          // don't indicate the versions.  The only way to differentiate is by the Chipset.
>          let (imem_sec_dst_start, imem_ns_load_target) = if chipset <= Chipset::GA100 {
>              (
> -                app0.offset,
> +                app0_code_offset,
>                  Some(FalconDmaLoadTarget {
>                      src_start: 0,
> -                    dst_start: load_hdr.os_code_offset,
> -                    len: load_hdr.os_code_size,
> +                    dst_start: os_code_offset,
> +                    len: os_code_size,
>                  }),
>              )
>          } else {
> @@ -378,15 +178,15 @@ pub(crate) fn new(
>  
>          Ok(Self {
>              imem_sec_load_target: FalconDmaLoadTarget {
> -                src_start: app0.offset,
> +                src_start: app0_code_offset,
>                  dst_start: imem_sec_dst_start,
> -                len: app0.len,
> +                len: app0_code_size,
>              },
>              imem_ns_load_target,
>              dmem_load_target: FalconDmaLoadTarget {
> -                src_start: load_hdr.os_data_offset,
> +                src_start: os_data_offset,
>                  dst_start: 0,
> -                len: load_hdr.os_data_size,
> +                len: os_data_size,
>              },
>              brom_params,
>              ucode: ucode_signed,
> diff --git a/drivers/gpu/nova-core/firmware/tlv.rs b/drivers/gpu/nova-core/firmware/tlv.rs
> index 56e0d5cab580..68b12637ff2c 100644
> --- a/drivers/gpu/nova-core/firmware/tlv.rs
> +++ b/drivers/gpu/nova-core/firmware/tlv.rs
> @@ -11,7 +11,6 @@
>  use crate::gpu;
>  
>  /// Requests the GPU firmware TLV `name` suitable for `chipset`.
> -#[expect(dead_code)]
>  pub(crate) fn request_tlv(
>      dev: &device::Device,
>      chipset: gpu::Chipset,
> @@ -117,6 +116,9 @@ fn next(&mut self) -> Option<Self::Item> {
>  /// be exactly partitionable into blocks (no trailing partial header or slack). After
>  /// that, [`TlvIter`] only signals end-of-stream via [`None`], not parse failure.
>  ///
> +/// Although the spec forbids duplicate tags, neither the constructor nor the iterator
> +/// enforces this restriction.  Instead, duplicate tags are simply ignored.
> +///

This chunk should probably be in patch 2.

>  /// # Invariants
>  ///
>  /// `data` is a validated TLV payload (the bytes *after* the `NVFW` magic): it is the exact
> @@ -129,7 +131,6 @@ pub(crate) struct Tlv<'a> {
>      data: &'a [u8],
>  }
>  
> -#[expect(dead_code)]
>  impl<'a> Tlv<'a> {
>      const MAGIC: &'static [u8; 4] = b"NVFW";
>  
> @@ -161,6 +162,7 @@ pub(crate) fn new(data: &'a [u8]) -> Result<Self> {
>              else {
>                  return Err(EINVAL);
>              };
> +

Same here.