Nouveau - Jun 2025 - [PATCH v4 18/20] gpu: nova-core: add types for patching firmware binaries

On Wed, May 21, 2025 at 03:45:13PM +0900, Alexandre Courbot
wrote:
> +/// A [`DmaObject`] containing a specific microcode ready to be loaded
into a falcon.
> +///
> +/// This is module-local and meant for sub-modules to use internally.
> +struct FirmwareDmaObject<F: FalconFirmware>(DmaObject,
PhantomData<F>);
> +
> +/// Trait for signatures to be patched directly into a given firmware.
> +///
> +/// This is module-local and meant for sub-modules to use internally.
> +trait FirmwareSignature<F: FalconFirmware>: AsRef<[u8]> {}
> +
> +#[expect(unused)]
> +impl<F: FalconFirmware> FirmwareDmaObject<F> {
> +    /// Creates a new `UcodeDmaObject` containing `data`.
> +    fn new(dev: &device::Device<device::Bound>, data: &[u8])
-> Result<Self> {
> +        DmaObject::from_data(dev, data).map(|dmaobj| Self(dmaobj,
PhantomData))
> +    }
> +
> +    /// Patches the firmware at offset `sig_base_img` with `signature`.
> +    fn patch_signature<S: FirmwareSignature<F>>(
> +        &mut self,
> +        signature: &S,
> +        sig_base_img: usize,
> +    ) -> Result<()> {
> +        let signature_bytes = signature.as_ref();
> +        if sig_base_img + signature_bytes.len() > self.0.size() {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: we are the only user of this object, so there cannot be
any race.
> +        let dst = unsafe { self.0.start_ptr_mut().add(sig_base_img) };
> +
> +        // SAFETY: `signature` and `dst` are valid, properly aligned, and
do not overlap.
> +        unsafe {
> +            core::ptr::copy_nonoverlapping(signature_bytes.as_ptr(), dst,
signature_bytes.len())
> +        };
> +
> +        Ok(())
> +    }
> +}
If we can't patch them when the object is created, i.e. in
FirmwareDmaObject::new(), I think we should take self by value in
FirmwareDmaObject::patch_signature() and return a SignedFirmwareDmaObject (which
can just be a transparent wrapper) instead in order to let the type system prove
that we did not forget to call patch_signature().

On Wed Jun 4, 2025 at 7:28 PM JST, Danilo Krummrich
wrote:
> On Wed, May 21, 2025 at 03:45:13PM +0900, Alexandre Courbot wrote:
>> +/// A [`DmaObject`] containing a specific microcode ready to be loaded
into a falcon.
>> +///
>> +/// This is module-local and meant for sub-modules to use internally.
>> +struct FirmwareDmaObject<F: FalconFirmware>(DmaObject,
PhantomData<F>);
>> +
>> +/// Trait for signatures to be patched directly into a given firmware.
>> +///
>> +/// This is module-local and meant for sub-modules to use internally.
>> +trait FirmwareSignature<F: FalconFirmware>: AsRef<[u8]> {}
>> +
>> +#[expect(unused)]
>> +impl<F: FalconFirmware> FirmwareDmaObject<F> {
>> +    /// Creates a new `UcodeDmaObject` containing `data`.
>> +    fn new(dev: &device::Device<device::Bound>, data:
&[u8]) -> Result<Self> {
>> +        DmaObject::from_data(dev, data).map(|dmaobj| Self(dmaobj,
PhantomData))
>> +    }
>> +
>> +    /// Patches the firmware at offset `sig_base_img` with
`signature`.
>> +    fn patch_signature<S: FirmwareSignature<F>>(
>> +        &mut self,
>> +        signature: &S,
>> +        sig_base_img: usize,
>> +    ) -> Result<()> {
>> +        let signature_bytes = signature.as_ref();
>> +        if sig_base_img + signature_bytes.len() > self.0.size() {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // SAFETY: we are the only user of this object, so there
cannot be any race.
>> +        let dst = unsafe { self.0.start_ptr_mut().add(sig_base_img) };
>> +
>> +        // SAFETY: `signature` and `dst` are valid, properly aligned,
and do not overlap.
>> +        unsafe {
>> +            core::ptr::copy_nonoverlapping(signature_bytes.as_ptr(),
dst, signature_bytes.len())
>> +        };
>> +
>> +        Ok(())
>> +    }
>> +}
>
> If we can't patch them when the object is created, i.e. in
> FirmwareDmaObject::new(), I think we should take self by value in
> FirmwareDmaObject::patch_signature() and return a SignedFirmwareDmaObject
(which
> can just be a transparent wrapper) instead in order to let the type system
prove
> that we did not forget to call patch_signature().
This one is a bit tricky. Signature patching is actually optional,
depending on whether there are signatures present at all (it might not
be the case on development setups). So involving the type system here
would require storing the result in an enum, and then match that enum
later in order to do the same thing in both cases - load the binary
as-is.

So I guess I would rather leave this one as it currently is, unless
there is a better way I haven't thought about?