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 <[email protected]>
> ---
> 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 <[email protected]>
