Re: [PATCH v10 01/10] gpu: nova-core: create falcon firmware DMA objects lazily

["Eliot Courtney" <ecourtney@nvidia.com>]

On Sun Mar 1, 2026 at 11:03 PM JST, Alexandre Courbot wrote:
> When DMA was the only loading option for falcon firmwares, we decided to
> store them in DMA objects as soon as they were loaded from disk and
> patch them in-place to avoid having to do an extra copy.
>
> This decision complicates the PIO loading patch considerably, and
> actually does not even stand on its own when put into perspective with
> the fact that it requires 8 unsafe statements in the code that wouldn't
> exist if we stored the firmware into a `KVVec` and copied it into a DMA
> object at the last minute.
>
> The cost of the copy is, as can be expected, imperceptible at runtime.
> Thus, switch to a lazy DMA object creation model and simplify our code
> a bit. This will also have the nice side-effect of being more fit for
> PIO loading.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon.rs          | 57 ++++++++++++-------
>  drivers/gpu/nova-core/firmware.rs        | 38 ++++++-------
>  drivers/gpu/nova-core/firmware/booter.rs | 33 +++++------
>  drivers/gpu/nova-core/firmware/fwsec.rs  | 96 ++++++++++----------------------
>  drivers/gpu/nova-core/gsp/boot.rs        |  2 +-
>  5 files changed, 99 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37bfee1d0949..8d444cf9d55c 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -2,12 +2,13 @@
>  
>  //! Falcon microprocessor base support
>  
> -use core::ops::Deref;
> -
>  use hal::FalconHal;
>  
>  use kernel::{
> -    device,
> +    device::{
> +        self,
> +        Device, //
> +    },
>      dma::{
>          DmaAddress,
>          DmaMask, //
> @@ -15,9 +16,7 @@
>      io::poll::read_poll_timeout,
>      prelude::*,
>      sync::aref::ARef,
> -    time::{
> -        Delta, //
> -    },
> +    time::Delta,

nit: Missing // guard here.

> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
> index df3d8de14ca1..9349c715a5a4 100644
> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
> @@ -10,10 +10,7 @@
>  //! - The command to be run, as this firmware can perform several tasks ;
>  //! - The ucode signature, so the GSP falcon can run FWSEC in HS mode.
>  
> -use core::{
> -    marker::PhantomData,
> -    ops::Deref, //
> -};
> +use core::marker::PhantomData;
>  
>  use kernel::{
>      device::{
> @@ -28,7 +25,6 @@
>  };
>  
>  use crate::{
> -    dma::DmaObject,
>      driver::Bar0,
>      falcon::{
>          gsp::Gsp,
> @@ -40,7 +36,7 @@
>      },
>      firmware::{
>          FalconUCodeDesc,
> -        FirmwareDmaObject,
> +        FirmwareObject,
>          FirmwareSignature,
>          Signed,
>          Unsigned, //
> @@ -174,52 +170,21 @@ fn as_ref(&self) -> &[u8] {
>  
>  impl FirmwareSignature<FwsecFirmware> for Bcrt30Rsa3kSignature {}
>  
> -/// Reinterpret the area starting from `offset` in `fw` as an instance of `T` (which must implement
> -/// [`FromBytes`]) and return a reference to it.
> -///
> -/// # Safety
> -///
> -/// * Callers must ensure that the device does not read/write to/from memory while the returned
> -///   reference is live.
> -/// * Callers must ensure that this call does not race with a write to the same region while
> -///   the returned reference is live.
> -unsafe fn transmute<T: Sized + FromBytes>(fw: &DmaObject, offset: usize) -> Result<&T> {
> -    // SAFETY: The safety requirements of the function guarantee the device won't read
> -    // or write to memory while the reference is alive and that this call won't race
> -    // with writes to the same memory region.
> -    T::from_bytes(unsafe { fw.as_slice(offset, size_of::<T>())? }).ok_or(EINVAL)
> -}
> -
> -/// Reinterpret the area starting from `offset` in `fw` as a mutable instance of `T` (which must
> -/// implement [`FromBytes`]) and return a reference to it.
> -///
> -/// # Safety
> -///
> -/// * Callers must ensure that the device does not read/write to/from memory while the returned
> -///   slice is live.
> -/// * Callers must ensure that this call does not race with a read or write to the same region
> -///   while the returned slice is live.
> -unsafe fn transmute_mut<T: Sized + FromBytes + AsBytes>(
> -    fw: &mut DmaObject,
> -    offset: usize,
> -) -> Result<&mut T> {
> -    // SAFETY: The safety requirements of the function guarantee the device won't read
> -    // or write to memory while the reference is alive and that this call won't race
> -    // with writes or reads to the same memory region.
> -    T::from_bytes_mut(unsafe { fw.as_slice_mut(offset, size_of::<T>())? }).ok_or(EINVAL)
> -}
> -
>  /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP falcon.
>  ///
>  /// It is responsible for e.g. carving out the WPR2 region as the first step of the GSP bootflow.
>  pub(crate) struct FwsecFirmware {
>      /// Descriptor of the firmware.
>      desc: FalconUCodeDesc,
> -    /// GPU-accessible DMA object containing the firmware.
> -    ucode: FirmwareDmaObject<Self, Signed>,
> +    /// Object containing the firmware binary.
> +    ucode: FirmwareObject<Self, Signed>,
>  }
>  
>  impl FalconLoadParams for FwsecFirmware {
> +    fn as_slice(&self) -> &[u8] {
> +        self.ucode.0.as_slice()
> +    }
> +
>      fn imem_sec_load_params(&self) -> FalconLoadTarget {
>          self.desc.imem_sec_load_params()
>      }
> @@ -245,23 +210,15 @@ fn boot_addr(&self) -> u32 {
>      }
>  }
>  
> -impl Deref for FwsecFirmware {
> -    type Target = DmaObject;
> -
> -    fn deref(&self) -> &Self::Target {
> -        &self.ucode.0
> -    }
> -}
> -
>  impl FalconFirmware for FwsecFirmware {
>      type Target = Gsp;
>  }
>  
> -impl FirmwareDmaObject<FwsecFirmware, Unsigned> {
> -    fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Result<Self> {
> +impl FirmwareObject<FwsecFirmware, Unsigned> {
> +    fn new_fwsec(bios: &Vbios, cmd: FwsecCommand) -> Result<Self> {
>          let desc = bios.fwsec_image().header()?;
> -        let ucode = bios.fwsec_image().ucode(&desc)?;
> -        let mut dma_object = DmaObject::from_data(dev, ucode)?;
> +        let mut ucode = KVVec::new();
> +        ucode.extend_from_slice(bios.fwsec_image().ucode(&desc)?, GFP_KERNEL)?;
>  
>          let hdr_offset = desc
>              .imem_load_size()
> @@ -269,8 +226,9 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>              .map(usize::from_safe_cast)
>              .ok_or(EINVAL)?;
>  
> -        // SAFETY: we have exclusive access to `dma_object`.
> -        let hdr: &FalconAppifHdrV1 = unsafe { transmute(&dma_object, hdr_offset) }?;
> +        let hdr = FalconAppifHdrV1::from_bytes_prefix(&ucode[hdr_offset..])
> +            .ok_or(EINVAL)?
> +            .0;

Is it worth adding // PANIC: comments like we have in some other areas
of the codebase for each of these indexes into ucode?

Other than those two optional nits,
Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>