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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Aug 27, 2025 at 11:29 AM JST, John Hubbard wrote:
> 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 "//" ?

Absolutely! Thanks.

>
> ...> +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:

Indeed, that block was a bit too dense. :)

>
> 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. :)

Technically it is of type `FirmwareDmaObject<Signed>` even if we take to
last branch. The name is to confirm that we have taken care of the
signature step (even if it is unneeded). Not sure what would be a better
name for this.

>
>> +            } 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.

Mmm, you're right - on the other hand I don't think I would have added a
HAL method unless I saw at least two different implementations in
OpenRM, but of course I am not 100% sure. Let's keep this in mind and
simplify it if we see it is indeed unneeded down the road.

>
>> +                    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;

Good idea.

>
>> +                    // 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?

If the data is out of bounds, it will be caught when trying to load the
firmware into the falcon engine. I am fine with adding a check here as
well if you think it it better.

>
>
>> +            },
>> +            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
>     }
> }

`Deref<Target = DmaObject>` is a requirement of `FalconFirmware`. That
being said, we could turn that into a `dma_object` method of
`FalconFirmware`, which might be a bit clearer about the intent...