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
