Re: [PATCH v2 3/8] gpu: nova-core: firmware: process Booter and patch its signature

[John Hubbard <jhubbard@nvidia.com>]

On 8/25/25 9:07 PM, Alexandre Courbot wrote:
...
> +/// Signature parameters, as defined in the firmware.
> +#[repr(C)]
> +struct HsSignatureParams {
> +    // Fuse version to use.
> +    fuse_ver: u32,
> +    // Mask of engine IDs this firmware applies to.
> +    engine_id_mask: u32,
> +    // ID of the microcode.

Should these three comments use "///" instead of "//" ?

...> +pub(crate) struct BooterFirmware {
> +    // Load parameters for `IMEM` falcon memory.
> +    imem_load_target: FalconLoadTarget,
> +    // Load parameters for `DMEM` falcon memory.
> +    dmem_load_target: FalconLoadTarget,
> +    // BROM falcon parameters.
> +    brom_params: FalconBromParams,
> +    // Device-mapped firmware image.
> +    ucode: FirmwareDmaObject<Self, Signed>,
> +}
> +
> +impl FirmwareDmaObject<BooterFirmware, Unsigned> {
> +    fn new_booter(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
> +        DmaObject::from_data(dev, data).map(|ucode| Self(ucode, PhantomData))
> +    }
> +}
> +
> +impl BooterFirmware {
> +    /// Parses the Booter firmware contained in `fw`, and patches the correct signature so it is
> +    /// ready to be loaded and run on `falcon`.
> +    pub(crate) fn new(
> +        dev: &device::Device<device::Bound>,
> +        fw: &Firmware,
> +        falcon: &Falcon<<Self as FalconFirmware>::Target>,
> +        bar: &Bar0,
> +    ) -> Result<Self> {
> +        let bin_fw = BinFirmware::new(fw)?;

A few newlines for a little visual "vertical relief" would be a
welcome break from this wall of text. Maybe one before and after
each comment+line, just for this one time here, if that's not too 
excessive:

here> +        // The binary firmware embeds a Heavy-Secured firmware.
> +        let hs_fw = HsFirmwareV2::new(&bin_fw)?;
here> +        // The Heavy-Secured firmware embeds a firmware load descriptor.
> +        let load_hdr = HsLoadHeaderV2::new(&hs_fw)?;
here> +        // Offset in `ucode` where to patch the signature.
> +        let patch_loc = hs_fw.patch_location()?;
here> +        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
> +            // 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)?,
> +        };
> +        let app0 = HsLoadHeaderV2App::new(&hs_fw, 0)?;
> +
> +        // Object containing the firmware microcode to be signature-patched.
> +        let ucode = bin_fw
> +            .data()
> +            .ok_or(EINVAL)
> +            .and_then(|data| FirmwareDmaObject::<Self, _>::new_booter(dev, data))?;
> +
> +        let ucode_signed = {

This ucode_signed variable is misnamed...

> +            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()

...as we can see here. :)

> +            } else {
> +                // Obtain the version from the fuse register, and extract the corresponding
> +                // signature.
> +                let reg_fuse_version = falcon.signature_reg_fuse_version(

Oh...I don't want to derail this patch review with a pre-existing problem,
but let me mention it anyway so I don't forget: .signature_reg_fuse_version()
appears to be unnecessarily HAL-ified. I think.

SEC2 boot flow only applies to Turing, Ampere, Ada, and so unless Timur
uncovers a Turing-specific signature_reg_fuse_version(), then I think
we'd best delete that entire HAL area and call it directly.

Again, nothing to do with this patch, I'm just looking for a quick
sanity check on my first reading of this situation.

> +                    bar,
> +                    brom_params.engine_id_mask,
> +                    brom_params.ucode_id,
> +                )?;
> +
> +                let signature = match reg_fuse_version {
> +                    // `0` means the last signature should be used.
> +                    0 => signatures.last(),

Should we provide a global const, to make this concept a little more self-documenting?
Approximately: 

const FUSE_VERSION_USE_LAST_SIG: u32 = 0;

> +                    // Otherwise hardware fuse version needs to be substracted to obtain the index.

typo: "s/substracted/subtracted/"

> +                    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 as usize)
> +                    }
> +                }
> +                .ok_or(EINVAL)?;
> +
> +                ucode.patch_signature(&signature, patch_loc as usize)?
> +            }
> +        };
> +
> +        Ok(Self {
> +            imem_load_target: FalconLoadTarget {
> +                src_start: app0.offset,
> +                dst_start: 0,
> +                len: app0.len,

Should we check that app0.offset.checked_add(app0.len) doesn't cause an
out of bounds read?


> +            },
> +            dmem_load_target: FalconLoadTarget {
> +                src_start: load_hdr.os_data_offset,
> +                dst_start: 0,
> +                len: load_hdr.os_data_size,
> +            },
> +            brom_params,
> +            ucode: ucode_signed,
> +        })
> +    }
> +}
> +
> +impl FalconLoadParams for BooterFirmware {
> +    fn imem_load_params(&self) -> FalconLoadTarget {
> +        self.imem_load_target.clone()
> +    }
> +
> +    fn dmem_load_params(&self) -> FalconLoadTarget {
> +        self.dmem_load_target.clone()
> +    }
> +
> +    fn brom_params(&self) -> FalconBromParams {
> +        self.brom_params.clone()
> +    }
> +
> +    fn boot_addr(&self) -> u32 {
> +        self.imem_load_target.src_start
> +    }
> +}
> +
> +impl Deref for BooterFirmware {
> +    type Target = DmaObject;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.ucode.0
> +    }
> +}

OK, so this allows &BooterFirmware to be used where &DmaObject is expected,
but it's not immediately obvious that BooterFirmware derefs to its internal
DMA object. It feels too clever...

Could we do something a little more obvious instead? Sort of like this:

impl BooterFirmware {
    pub(crate) fn dma_object(&self) -> &DmaObject {
        &self.ucode.0
    }
}

...

I'm out of time today, will work on the other half of the series tomorrow.

thanks,
-- 
John Hubbard