Nouveau - Aug 2025 - [PATCH v2 2/8] gpu: nova-core: firmware: add support for common firmware header

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 <acourbot at 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

On Wed Aug 27, 2025 at 10:34 AM JST, John Hubbard wrote:
<snip>
>> +    /// 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?
`get` returns `None` if the requested slice is out of bounds, so there
should be no risk of panicking here.

However, `fw_start + fw_size` can panic in debug configuration if it
overflows. In a release build I believe it will just happily wrap, and
`get` should consequently return `None` at the invalid range... Although
we can also get unlucky and produce a valid, yet incorrect, one.

This is actually something I've been thinking about while writing this
series and could not really decide upon: how to deal with operands and
functions in Rust that can potentially panic. Using `checked` operands
everywhere is a bit tedious, and even with great care there is no way to
guarantee that no panic occurs in a given function.

Panics are a big no-no in the kernel, yet I don't feel like we have the
proper tools to ensure they do not happen.

User-space has some crates like `no_panic`, but even these feel more
like hacks than anything else. Something at the compiler level would be
nice.

Maybe that would be a good discussion topic for the Plumber
Microconference?