On 8/25/25 9:07 PM, Alexandre Courbot wrote:
> Several firmware files loaded from userspace feature a common header
> that describes their payload. Add basic support for it so subsequent
> patches can leverage it.
>
> Signed-off-by: Alexandre Courbot <acour...@nvidia.com>
> ---
> drivers/gpu/nova-core/firmware.rs | 62
> +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/firmware.rs
> b/drivers/gpu/nova-core/firmware.rs
> index
> 2931912ddba0ea1fe6d027ccec70b39cdb40344a..ccb4d19f8fa76b0e844252dede5f50b37c590571
> 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -4,11 +4,13 @@
> //! to be loaded into a given execution unit.
>
> use core::marker::PhantomData;
> +use core::mem::size_of;
>
> use kernel::device;
> use kernel::firmware;
> use kernel::prelude::*;
> use kernel::str::CString;
> +use kernel::transmute::FromBytes;
>
> use crate::dma::DmaObject;
> use crate::falcon::FalconFirmware;
> @@ -150,6 +152,66 @@ fn no_patch_signature(self) -> FirmwareDmaObject<F,
> Signed> {
> }
> }
>
> +/// Header common to most firmware files.
> +#[repr(C)]
> +#[derive(Debug, Clone)]
> +struct BinHdr {
> + /// Magic number, must be `0x10de`.
How about:
/// Magic number, required to be equal to BIN_MAGIC
...and see below.
> + bin_magic: u32,
> + /// Version of the header.
> + bin_ver: u32,
> + /// Size in bytes of the binary (to be ignored).
> + bin_size: u32,
> + /// Offset of the start of the application-specific header.
> + header_offset: u32,
> + /// Offset of the start of the data payload.
> + data_offset: u32,
> + /// Size in bytes of the data payload.
> + data_size: u32,
> +}
> +
> +// SAFETY: all bit patterns are valid for this type, and it doesn't use
> interior mutability.
> +unsafe impl FromBytes for BinHdr {}
> +
> +// A firmware blob starting with a `BinHdr`.
> +struct BinFirmware<'a> {
> + hdr: BinHdr,
> + 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.
> + fn new(fw: &'a firmware::Firmware) -> Result<Self> {
> + const BIN_MAGIC: u32 = 0x10de;
Let's promote this to approximately file scope and put it just
above the BinHdr definition. Then we end up with one definition
at the ideal scope, and the comment docs take better care of
themselves.
> + let fw = fw.data();
> +
> + fw.get(0..size_of::<BinHdr>())
> + // Extract header.
> + .and_then(BinHdr::from_bytes_copy)
> + // Validate header.
> + .and_then(|hdr| {
> + if hdr.bin_magic == BIN_MAGIC {
> + Some(hdr)
> + } else {
> + None
> + }
> + })
> + .map(|hdr| Self { hdr, fw })
> + .ok_or(EINVAL)
> + }
> +
> + /// Returns the data payload of the firmware, or `None` if the data
> range is out of bounds of
> + /// the firmware image.
> + fn data(&self) -> Option<&[u8]> {
> + let fw_start = self.hdr.data_offset as usize;
> + let fw_size = self.hdr.data_size as usize;
> +
> + self.fw.get(fw_start..fw_start + fw_size)
This worries me a bit, because we never checked that these bounds
are reasonable: within the range of the firmware, and not overflowing
(.checked_add() for example), that sort of thing.
Thoughts?
> + }
> +}
> +
> pub(crate) struct ModInfoBuilder<const N:
> usize>(firmware::ModInfoBuilder<N>);
>
> impl<const N: usize> ModInfoBuilder<N> {
>
thanks,
--
John Hubbard
