The Booter signed firmware is an essential part of bringing up the GSP
on Turing and Ampere. It is loaded on the sec2 falcon core and is
responsible for loading and running the RISC-V GSP bootloader into the
GSP core.
Add support for parsing the Booter firmware loaded from userspace, patch
its signatures, and store it into a form that is ready to be loaded and
executed on the sec2 falcon.
We do not run it yet, as its own payload (the GSP bootloader and
firmware image) still need to be prepared.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/gpu/nova-core/falcon.rs | 4 +-
drivers/gpu/nova-core/firmware.rs | 25 ++-
drivers/gpu/nova-core/firmware/booter.rs | 356 +++++++++++++++++++++++++++++++
drivers/gpu/nova-core/gpu.rs | 11 +-
4 files changed, 386 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 2dbcdf26697beb7e52083675fc9ea62a6167fef8..7bd13481a6a37783309c2d2621a6b67b81d55cc5 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -293,7 +293,7 @@ pub(crate) trait FalconEngine:
}
/// Represents a portion of the firmware to be loaded into a particular memory (e.g. IMEM or DMEM).
-#[derive(Debug)]
+#[derive(Debug, Clone)]
pub(crate) struct FalconLoadTarget {
/// Offset from the start of the source object to copy from.
pub(crate) src_start: u32,
@@ -304,7 +304,7 @@ pub(crate) struct FalconLoadTarget {
}
/// Parameters for the falcon boot ROM.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
pub(crate) struct FalconBromParams {
/// Offset in `DMEM`` of the firmware's signature.
pub(crate) pkc_data_offset: u32,
diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
index ccb4d19f8fa76b0e844252dede5f50b37c590571..be190af1e11aec26c18c85324a185d135a16eabe 100644
--- a/drivers/gpu/nova-core/firmware.rs
+++ b/drivers/gpu/nova-core/firmware.rs
@@ -6,6 +6,7 @@
use core::marker::PhantomData;
use core::mem::size_of;
+use booter::BooterFirmware;
use kernel::device;
use kernel::firmware;
use kernel::prelude::*;
@@ -13,10 +14,13 @@
use kernel::transmute::FromBytes;
use crate::dma::DmaObject;
+use crate::driver::Bar0;
use crate::falcon::FalconFirmware;
+use crate::falcon::{sec2::Sec2, Falcon};
use crate::gpu;
use crate::gpu::Chipset;
+pub(crate) mod booter;
pub(crate) mod fwsec;
pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
@@ -24,14 +28,22 @@
/// Structure encapsulating the firmware blobs required for the GPU to operate.
#[expect(dead_code)]
pub(crate) struct Firmware {
- booter_load: firmware::Firmware,
- booter_unload: firmware::Firmware,
+ /// Runs on the sec2 falcon engine to load and start the GSP bootloader.
+ booter_loader: BooterFirmware,
+ /// Runs on the sec2 falcon engine to stop and unload a running GSP firmware.
+ booter_unloader: BooterFirmware,
bootloader: firmware::Firmware,
gsp: firmware::Firmware,
}
impl Firmware {
- pub(crate) fn new(dev: &device::Device, chipset: Chipset, ver: &str) -> Result<Firmware> {
+ pub(crate) fn new(
+ dev: &device::Device<device::Bound>,
+ sec2: &Falcon<Sec2>,
+ bar: &Bar0,
+ chipset: Chipset,
+ ver: &str,
+ ) -> Result<Firmware> {
let mut chip_name = CString::try_from_fmt(fmt!("{chipset}"))?;
chip_name.make_ascii_lowercase();
let chip_name = &*chip_name;
@@ -42,8 +54,10 @@ pub(crate) fn new(dev: &device::Device, chipset: Chipset, ver: &str) -> Result<F
};
Ok(Firmware {
- booter_load: request("booter_load")?,
- booter_unload: request("booter_unload")?,
+ booter_loader: request("booter_load")
+ .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
+ booter_unloader: request("booter_unload")
+ .and_then(|fw| BooterFirmware::new(dev, &fw, sec2, bar))?,
bootloader: request("bootloader")?,
gsp: request("gsp")?,
})
@@ -179,7 +193,6 @@ struct BinFirmware<'a> {
fw: &'a [u8],
}
-#[expect(dead_code)]
impl<'a> BinFirmware<'a> {
/// Interpret `fw` as a firmware image starting with a [`BinHdr`], and returns the
/// corresponding [`BinFirmware`] that can be used to extract its payload.
diff --git a/drivers/gpu/nova-core/firmware/booter.rs b/drivers/gpu/nova-core/firmware/booter.rs
new file mode 100644
index 0000000000000000000000000000000000000000..108649bdf716eeacaae3098b3c29b2de2813c6ee
--- /dev/null
+++ b/drivers/gpu/nova-core/firmware/booter.rs
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Support for loading and patching the `Booter` firmware. `Booter` is a Heavy Secured firmware
+//! running on [`Sec2`], that is used on Turing/Ampere to load the GSP firmware into the GSP falcon
+//! (and optionally unload it through a separate firmware image).
+
+use core::marker::PhantomData;
+use core::mem::size_of;
+use core::ops::Deref;
+
+use kernel::device;
+use kernel::firmware::Firmware;
+use kernel::prelude::*;
+use kernel::transmute::FromBytes;
+
+use crate::dma::DmaObject;
+use crate::driver::Bar0;
+use crate::falcon::sec2::Sec2;
+use crate::falcon::{Falcon, FalconBromParams, FalconFirmware, FalconLoadParams, FalconLoadTarget};
+use crate::firmware::{BinFirmware, FirmwareDmaObject, FirmwareSignature, Signed, Unsigned};
+
+/// Local convenience function to return a copy of `S` by reinterpreting the bytes starting at
+/// `offset` in `slice`.
+fn frombytes_at<S: FromBytes + Sized>(slice: &[u8], offset: usize) -> Result<S> {
+ slice
+ .get(offset..offset + size_of::<S>())
+ .and_then(S::from_bytes_copy)
+ .ok_or(EINVAL)
+}
+
+/// Heavy-Secured firmware header.
+///
+/// Such firmwares have an application-specific payload that needs to be patched with a given
+/// signature.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct HsHeaderV2 {
+ /// Offset to the start of the signatures.
+ sig_prod_offset: u32,
+ /// Size in bytes of the signatures.
+ sig_prod_size: u32,
+ /// Offset to a `u32` containing the location at which to patch the signature in the microcode
+ /// image.
+ patch_loc: u32,
+ /// Offset to a `u32` containing the index of the signature to patch.
+ patch_sig: u32,
+ /// Start offset to the signature metadata.
+ meta_data_offset: u32,
+ /// Size in bytes of the signature metadata.
+ meta_data_size: u32,
+ /// Offset to a `u32` containing the number of signatures in the signatures section.
+ num_sig: u32,
+ /// Offset of the application-specific header.
+ header_offset: u32,
+ /// Size in bytes of the application-specific header.
+ header_size: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsHeaderV2 {}
+
+/// Heavy-Secured Firmware image container.
+///
+/// This provides convenient access to the fields of [`HsHeaderV2`] that are actually indices to
+/// read from in the firmware data.
+struct HsFirmwareV2<'a> {
+ hdr: HsHeaderV2,
+ fw: &'a [u8],
+}
+
+impl<'a> HsFirmwareV2<'a> {
+ /// Interprets the header of `bin_fw` as a [`HsHeaderV2`] and returns an instance of
+ /// `HsFirmwareV2` for further parsing.
+ ///
+ /// Fails if the header pointed at by `bin_fw` is not within the bounds of the firmware image.
+ fn new(bin_fw: &BinFirmware<'a>) -> Result<Self> {
+ frombytes_at::<HsHeaderV2>(bin_fw.fw, bin_fw.hdr.header_offset as usize)
+ .map(|hdr| Self { hdr, fw: bin_fw.fw })
+ }
+
+ /// Returns the location at which the signatures should be patched in the microcode image.
+ ///
+ /// Fails if the offset of the patch location is outside the bounds of the firmware
+ /// image.
+ fn patch_location(&self) -> Result<u32> {
+ frombytes_at::<u32>(self.fw, self.hdr.patch_loc as usize)
+ }
+
+ /// Returns an iterator to the signatures of the firmware. The iterator can be empty if the
+ /// firmware is unsigned.
+ ///
+ /// Fails if the pointed signatures are outside the bounds of the firmware image.
+ fn signatures_iter(&'a self) -> Result<impl Iterator<Item = BooterSignature<'a>>> {
+ let num_sig = frombytes_at::<u32>(self.fw, self.hdr.num_sig as usize)?;
+ let iter = match self.hdr.sig_prod_size.checked_div(num_sig) {
+ // If there are no signatures, return an iterator that will yield zero elements.
+ None => (&[] as &[u8]).chunks_exact(1),
+ Some(sig_size) => {
+ let patch_sig = frombytes_at::<u32>(self.fw, self.hdr.patch_sig as usize)?;
+ let signatures_start = (self.hdr.sig_prod_offset + patch_sig) as usize;
+
+ self.fw
+ // Get signatures range.
+ .get(signatures_start..signatures_start + self.hdr.sig_prod_size as usize)
+ .ok_or(EINVAL)?
+ .chunks_exact(sig_size as usize)
+ }
+ };
+
+ // Map the byte slices into signatures.
+ Ok(iter.map(BooterSignature))
+ }
+}
+
+/// 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.
+ ucode_id: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsSignatureParams {}
+
+impl HsSignatureParams {
+ /// Returns the signature parameters contained in `hs_fw`.
+ ///
+ /// Fails if the meta data parameter of `hs_fw` is outside the bounds of the firmware image, or
+ /// if its size doesn't match that of [`HsSignatureParams`].
+ fn new(hs_fw: &HsFirmwareV2<'_>) -> Result<Self> {
+ let start = hs_fw.hdr.meta_data_offset as usize;
+ let end = start
+ .checked_add(hs_fw.hdr.meta_data_size as usize)
+ .ok_or(EINVAL)?;
+
+ hs_fw
+ .fw
+ .get(start..end)
+ .and_then(Self::from_bytes_copy)
+ .ok_or(EINVAL)
+ }
+}
+
+/// Header for code and data load offsets.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct HsLoadHeaderV2 {
+ // Offset at which the code starts.
+ os_code_offset: u32,
+ // Total size of the code, for all apps.
+ os_code_size: u32,
+ // Offset at which the data starts.
+ os_data_offset: u32,
+ // Size of the data.
+ os_data_size: u32,
+ // Number of apps following this header. Each app is described by a [`HsLoadHeaderV2App`].
+ num_apps: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsLoadHeaderV2 {}
+
+impl HsLoadHeaderV2 {
+ /// Returns the load header contained in `hs_fw`.
+ ///
+ /// Fails if the header pointed at by `hs_fw` is not within the bounds of the firmware image.
+ fn new(hs_fw: &HsFirmwareV2<'_>) -> Result<Self> {
+ frombytes_at::<Self>(hs_fw.fw, hs_fw.hdr.header_offset as usize)
+ }
+}
+
+/// Header for app code loader.
+#[repr(C)]
+#[derive(Debug, Clone)]
+struct HsLoadHeaderV2App {
+ /// Offset at which to load the app code.
+ offset: u32,
+ /// Length in bytes of the app code.
+ len: u32,
+}
+
+// SAFETY: all bit patterns are valid for this type, and it doesn't use interior mutability.
+unsafe impl FromBytes for HsLoadHeaderV2App {}
+
+impl HsLoadHeaderV2App {
+ /// Returns the [`HsLoadHeaderV2App`] for app `idx` of `hs_fw`.
+ ///
+ /// Fails if `idx` is larger than the number of apps declared in `hs_fw`, or if the header is
+ /// not within the bounds of the firmware image.
+ fn new(hs_fw: &HsFirmwareV2<'_>, idx: u32) -> Result<Self> {
+ let load_hdr = HsLoadHeaderV2::new(hs_fw)?;
+ if idx >= load_hdr.num_apps {
+ Err(EINVAL)
+ } else {
+ frombytes_at::<Self>(
+ hs_fw.fw,
+ (hs_fw.hdr.header_offset as usize)
+ // Skip the load header...
+ .checked_add(size_of::<HsLoadHeaderV2>())
+ // ... and jump to app header `idx`.
+ .and_then(|offset| {
+ offset.checked_add((idx as usize).checked_mul(size_of::<Self>())?)
+ })
+ .ok_or(EINVAL)?,
+ )
+ }
+ }
+}
+
+/// 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]);
+
+impl<'a> AsRef<[u8]> for BooterSignature<'a> {
+ fn as_ref(&self) -> &[u8] {
+ self.0
+ }
+}
+
+impl<'a> FirmwareSignature<BooterFirmware> for BooterSignature<'a> {}
+
+/// The `Booter` loader firmware, responsible for loading the GSP.
+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)?;
+ // 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 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 = {
+ 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(
+ 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(),
+ // Otherwise hardware fuse version needs to be substracted 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 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,
+ },
+ 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
+ }
+}
+
+impl FalconFirmware for BooterFirmware {
+ type Target = Sec2;
+}
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 8caecaf7dfb4820a96a568a05653dbdf808a3719..54f0e9fd587ae5c4c045096930c0548fb1ef1b86 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -269,7 +269,6 @@ pub(crate) fn new(
) -> Result<impl PinInit<Self>> {
let bar = devres_bar.access(pdev.as_ref())?;
let spec = Spec::new(bar)?;
- let fw = Firmware::new(pdev.as_ref(), spec.chipset, FIRMWARE_VERSION)?;
dev_info!(
pdev.as_ref(),
@@ -293,7 +292,15 @@ pub(crate) fn new(
)?;
gsp_falcon.clear_swgen0_intr(bar);
- let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
+ let sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
+
+ let fw = Firmware::new(
+ pdev.as_ref(),
+ &sec2_falcon,
+ bar,
+ spec.chipset,
+ FIRMWARE_VERSION,
+ )?;
let fb_layout = FbLayout::new(spec.chipset, bar)?;
dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout);
--
2.50.1On Tue, 2025-08-26 at 13:07 +0900, Alexandre Courbot wrote:
> +struct HsHeaderV2 {
> + /// Offset to the start of the signatures.
> + sig_prod_offset: u32,
> + /// Size in bytes of the signatures.
> + sig_prod_size: u32,
> + /// Offset to a `u32` containing the location at which to patch the signature in the
> microcode
> + /// image.
> + patch_loc: u32,
> + /// Offset to a `u32` containing the index of the signature to patch.
> + patch_sig: u32,
> + /// Start offset to the signature metadata.
> + meta_data_offset: u32,
> + /// Size in bytes of the signature metadata.
> + meta_data_size: u32,
> + /// Offset to a `u32` containing the number of signatures in the signatures section.
> + num_sig: u32,
> + /// Offset of the application-specific header.
> + header_offset: u32,
> + /// Size in bytes of the application-specific header.
> + header_size: u32,
> +}
You are inconsistent with the names of offset fields in this struct.
patch_loc should be patch_loc_offset
patch_sig should be patch_sig_offset
num_sig should be num_sig_offset
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
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...
On 8/28/25 12:19 AM, Alexandre Courbot wrote:
> On Wed Aug 27, 2025 at 11:29 AM JST, John Hubbard wrote:
>> On 8/25/25 9:07 PM, Alexandre Courbot wrote:
>> ...
>>> + 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.
So the <Signed> parameter naming is also awkward, because it refers
to non-signed firmware too, I see. So we need to rename both.
Ideas:
a) ucode_maybe_signed, FirmwareDmaObject<MaybeSigned>
b) ucode_validated, FirmwareDmaObject<Validated>
...
>> 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...
That does seem clearer to me.
thanks,
--
John Hubbard
© 2016 - 2025 Red Hat, Inc.