[PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Daniel del Castillo]

This patch solves the existing mentions of COHA, a task
in the Nova task list about improving the `CoherentAllocation` API.
I confirmed by talking to Alexandre Courbot, that the reading/writing
methods in `CoherentAllocation` can never be safe, so
this patch doesn't actually change `CoherentAllocation`, but rather
tries to solve the existing references to [COHA].

Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@gmail.com>
---
 drivers/gpu/nova-core/dma.rs            |  20 ++---
 drivers/gpu/nova-core/firmware/fwsec.rs | 104 ++++++++++--------------
 2 files changed, 50 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
index 94f44bcfd748..639a99cf72c4 100644
--- a/drivers/gpu/nova-core/dma.rs
+++ b/drivers/gpu/nova-core/dma.rs
@@ -25,21 +25,11 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
     }
 
     pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
-        Self::new(dev, data.len()).map(|mut dma_obj| {
-            // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
-            // SAFETY:
-            // - `dma_obj`'s size is at least `data.len()`.
-            // - We have just created this object and there is no other user at this stage.
-            unsafe {
-                core::ptr::copy_nonoverlapping(
-                    data.as_ptr(),
-                    dma_obj.dma.start_ptr_mut(),
-                    data.len(),
-                );
-            }
-
-            dma_obj
-        })
+        let mut dma_obj = Self::new(dev, data.len())?;
+        // SAFETY: We have just created this object and there is no other user at this stage.
+        unsafe { dma_obj.write(data, 0)? };
+
+        Ok(dma_obj)
     }
 }
 
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
index 8edbb5c0572c..cc5a6200d0de 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -11,12 +11,12 @@
 //! - The ucode signature, so the GSP falcon can run FWSEC in HS mode.
 
 use core::marker::PhantomData;
-use core::mem::{align_of, size_of};
+use core::mem::size_of;
 use core::ops::Deref;
 
 use kernel::device::{self, Device};
 use kernel::prelude::*;
-use kernel::transmute::FromBytes;
+use kernel::transmute::{AsBytes, FromBytes};
 
 use crate::dma::DmaObject;
 use crate::driver::Bar0;
@@ -70,6 +70,8 @@ struct FalconAppifDmemmapperV3 {
 }
 // SAFETY: any byte sequence is valid for this struct.
 unsafe impl FromBytes for FalconAppifDmemmapperV3 {}
+// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
+unsafe impl AsBytes for FalconAppifDmemmapperV3 {}
 
 #[derive(Debug)]
 #[repr(C, packed)]
@@ -82,6 +84,8 @@ struct ReadVbios {
 }
 // SAFETY: any byte sequence is valid for this struct.
 unsafe impl FromBytes for ReadVbios {}
+// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
+unsafe impl AsBytes for ReadVbios {}
 
 #[derive(Debug)]
 #[repr(C, packed)]
@@ -94,6 +98,8 @@ struct FrtsRegion {
 }
 // SAFETY: any byte sequence is valid for this struct.
 unsafe impl FromBytes for FrtsRegion {}
+// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
+unsafe impl AsBytes for FrtsRegion {}
 
 const NVFW_FRTS_CMD_REGION_TYPE_FB: u32 = 2;
 
@@ -104,6 +110,8 @@ struct FrtsCmd {
 }
 // SAFETY: any byte sequence is valid for this struct.
 unsafe impl FromBytes for FrtsCmd {}
+// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
+unsafe impl AsBytes for FrtsCmd {}
 
 const NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS: u32 = 0x15;
 const NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB: u32 = 0x19;
@@ -149,24 +157,9 @@ impl FirmwareSignature<FwsecFirmware> for Bcrt30Rsa3kSignature {}
 ///
 /// Callers must ensure that the region of memory returned is not written for as long as the
 /// returned reference is alive.
-///
-/// TODO[TRSM][COHA]: Remove this and `transmute_mut` once `CoherentAllocation::as_slice` is
-/// available and we have a way to transmute objects implementing FromBytes, e.g.:
-/// https://lore.kernel.org/lkml/20250330234039.29814-1-christiansantoslima21@gmail.com/
-unsafe fn transmute<'a, 'b, T: Sized + FromBytes>(
-    fw: &'a DmaObject,
-    offset: usize,
-) -> Result<&'b T> {
-    if offset + size_of::<T>() > fw.size() {
-        return Err(EINVAL);
-    }
-    if (fw.start_ptr() as usize + offset) % align_of::<T>() != 0 {
-        return Err(EINVAL);
-    }
-
-    // SAFETY: we have checked that the pointer is properly aligned that its pointed memory is
-    // large enough the contains an instance of `T`, which implements `FromBytes`.
-    Ok(unsafe { &*(fw.start_ptr().add(offset).cast::<T>()) })
+unsafe fn transmute<T: Sized + FromBytes>(fw: &DmaObject, offset: usize) -> Result<&T> {
+    // SAFETY: Guaranteed by the safety requirements of the function.
+    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
@@ -176,20 +169,12 @@ unsafe fn transmute<'a, 'b, T: Sized + FromBytes>(
 ///
 /// Callers must ensure that the region of memory returned is not read or written for as long as
 /// the returned reference is alive.
-unsafe fn transmute_mut<'a, 'b, T: Sized + FromBytes>(
-    fw: &'a mut DmaObject,
+unsafe fn transmute_mut<T: Sized + FromBytes + AsBytes>(
+    fw: &mut DmaObject,
     offset: usize,
-) -> Result<&'b mut T> {
-    if offset + size_of::<T>() > fw.size() {
-        return Err(EINVAL);
-    }
-    if (fw.start_ptr_mut() as usize + offset) % align_of::<T>() != 0 {
-        return Err(EINVAL);
-    }
-
-    // SAFETY: we have checked that the pointer is properly aligned that its pointed memory is
-    // large enough the contains an instance of `T`, which implements `FromBytes`.
-    Ok(unsafe { &mut *(fw.start_ptr_mut().add(offset).cast::<T>()) })
+) -> Result<&mut T> {
+    // SAFETY: Guaranteed by the safety requirements of the function.
+    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.
@@ -260,32 +245,38 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
 
         // Find the DMEM mapper section in the firmware.
         for i in 0..hdr.entry_count as usize {
-            let app: &FalconAppifV1 =
             // SAFETY: we have exclusive access to `dma_object`.
-            unsafe {
+            let app: &FalconAppifV1 = unsafe {
                 transmute(
                     &dma_object,
-                    hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize
+                    hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize,
                 )
             }?;
 
             if app.id != NVFW_FALCON_APPIF_ID_DMEMMAPPER {
                 continue;
             }
+            let dmem_base = app.dmem_base;
 
             // SAFETY: we have exclusive access to `dma_object`.
             let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
-                transmute_mut(
-                    &mut dma_object,
-                    (desc.imem_load_size + app.dmem_base) as usize,
-                )
+                transmute_mut(&mut dma_object, (desc.imem_load_size + dmem_base) as usize)
             }?;
 
+            dmem_mapper.init_cmd = match cmd {
+                FwsecCommand::Frts {
+                    frts_addr: _,
+                    frts_size: _,
+                } => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS,
+                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
+            };
+            let cmd_in_buffer_offset = dmem_mapper.cmd_in_buffer_offset;
+
             // SAFETY: we have exclusive access to `dma_object`.
             let frts_cmd: &mut FrtsCmd = unsafe {
                 transmute_mut(
                     &mut dma_object,
-                    (desc.imem_load_size + dmem_mapper.cmd_in_buffer_offset) as usize,
+                    (desc.imem_load_size + cmd_in_buffer_offset) as usize,
                 )
             }?;
 
@@ -296,24 +287,19 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
                 size: 0,
                 flags: 2,
             };
-
-            dmem_mapper.init_cmd = match cmd {
-                FwsecCommand::Frts {
-                    frts_addr,
-                    frts_size,
-                } => {
-                    frts_cmd.frts_region = FrtsRegion {
-                        ver: 1,
-                        hdr: size_of::<FrtsRegion>() as u32,
-                        addr: (frts_addr >> 12) as u32,
-                        size: (frts_size >> 12) as u32,
-                        ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
-                    };
-
-                    NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS
-                }
-                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
-            };
+            if let FwsecCommand::Frts {
+                frts_addr,
+                frts_size,
+            } = cmd
+            {
+                frts_cmd.frts_region = FrtsRegion {
+                    ver: 1,
+                    hdr: size_of::<FrtsRegion>() as u32,
+                    addr: (frts_addr >> 12) as u32,
+                    size: (frts_size >> 12) as u32,
+                    ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
+                };
+            }
 
             // Return early as we found and patched the DMEMMAPPER region.
             return Ok(Self(dma_object, PhantomData));
-- 
2.51.0

[PATCH 2/2] Update the nova todo list

[Daniel del Castillo]

This small patch updates the nova todo list to
remove some tasks that have been solved lately.

Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@gmail.com>
---
 Documentation/gpu/nova/core/todo.rst | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
index 48b20656dcb1..be8063030d44 100644
--- a/Documentation/gpu/nova/core/todo.rst
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -44,25 +44,6 @@ automatically generates the corresponding mappings between a value and a number.
 | Complexity: Beginner
 | Link: https://docs.rs/num/latest/num/trait.FromPrimitive.html
 
-Conversion from byte slices for types implementing FromBytes [TRSM]
--------------------------------------------------------------------
-
-We retrieve several structures from byte streams coming from the BIOS or loaded
-firmware. At the moment converting the bytes slice into the proper type require
-an inelegant `unsafe` operation; this will go away once `FromBytes` implements
-a proper `from_bytes` method.
-
-| Complexity: Beginner
-
-CoherentAllocation improvements [COHA]
---------------------------------------
-
-`CoherentAllocation` needs a safe way to write into the allocation, and to
-obtain slices within the allocation.
-
-| Complexity: Beginner
-| Contact: Abdiel Janulgue
-
 Generic register abstraction [REGA]
 -----------------------------------
 
-- 
2.51.0

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Danilo Krummrich]

Hi Daniel,

On 10/15/25 9:49 PM, Daniel del Castillo wrote:
> This patch solves the existing mentions of COHA, a task
> in the Nova task list about improving the `CoherentAllocation` API.
> I confirmed by talking to Alexandre Courbot, that the reading/writing
> methods in `CoherentAllocation` can never be safe, so
> this patch doesn't actually change `CoherentAllocation`, but rather
> tries to solve the existing references to [COHA].

This patch seems to address two different TODOs. Please split up the patch
accordingly.

> Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@gmail.com>
> ---
>  drivers/gpu/nova-core/dma.rs            |  20 ++---
>  drivers/gpu/nova-core/firmware/fwsec.rs | 104 ++++++++++--------------
>  2 files changed, 50 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
> index 94f44bcfd748..639a99cf72c4 100644
> --- a/drivers/gpu/nova-core/dma.rs
> +++ b/drivers/gpu/nova-core/dma.rs
> @@ -25,21 +25,11 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
>      }
>  
>      pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
> -        Self::new(dev, data.len()).map(|mut dma_obj| {
> -            // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
> -            // SAFETY:
> -            // - `dma_obj`'s size is at least `data.len()`.
> -            // - We have just created this object and there is no other user at this stage.
> -            unsafe {
> -                core::ptr::copy_nonoverlapping(
> -                    data.as_ptr(),
> -                    dma_obj.dma.start_ptr_mut(),
> -                    data.len(),
> -                );
> -            }
> -
> -            dma_obj
> -        })
> +        let mut dma_obj = Self::new(dev, data.len())?;
> +        // SAFETY: We have just created this object and there is no other user at this stage.

The safety comment should rather confirm that it is guaranteed that the device
won't access this memory concurrently.

> +        unsafe { dma_obj.write(data, 0)? };
> +
> +        Ok(dma_obj)
>      }
>  }
>  
> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
> index 8edbb5c0572c..cc5a6200d0de 100644
> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
> @@ -11,12 +11,12 @@
>  //! - The ucode signature, so the GSP falcon can run FWSEC in HS mode.
>  
>  use core::marker::PhantomData;
> -use core::mem::{align_of, size_of};
> +use core::mem::size_of;
>  use core::ops::Deref;
>  
>  use kernel::device::{self, Device};
>  use kernel::prelude::*;
> -use kernel::transmute::FromBytes;
> +use kernel::transmute::{AsBytes, FromBytes};
>  
>  use crate::dma::DmaObject;
>  use crate::driver::Bar0;
> @@ -70,6 +70,8 @@ struct FalconAppifDmemmapperV3 {
>  }
>  // SAFETY: any byte sequence is valid for this struct.
>  unsafe impl FromBytes for FalconAppifDmemmapperV3 {}
> +// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
> +unsafe impl AsBytes for FalconAppifDmemmapperV3 {}
>  
>  #[derive(Debug)]
>  #[repr(C, packed)]
> @@ -82,6 +84,8 @@ struct ReadVbios {
>  }
>  // SAFETY: any byte sequence is valid for this struct.
>  unsafe impl FromBytes for ReadVbios {}
> +// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.

The full safety requirement is "Values of this type may not contain any
uninitialized bytes. This type must not have interior mutability.

Additional NIT: Please start sentences with a capital letter (Unfortunately the
comment above missed this as well).

Same for the cases below.

> +unsafe impl AsBytes for ReadVbios {}
>  
>  #[derive(Debug)]
>  #[repr(C, packed)]
> @@ -94,6 +98,8 @@ struct FrtsRegion {
>  }
>  // SAFETY: any byte sequence is valid for this struct.
>  unsafe impl FromBytes for FrtsRegion {}
> +// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
> +unsafe impl AsBytes for FrtsRegion {}
>  
>  const NVFW_FRTS_CMD_REGION_TYPE_FB: u32 = 2;
>  
> @@ -104,6 +110,8 @@ struct FrtsCmd {
>  }
>  // SAFETY: any byte sequence is valid for this struct.
>  unsafe impl FromBytes for FrtsCmd {}
> +// SAFETY: any byte sequence is valid and it contains no padding nor kernel pointers.
> +unsafe impl AsBytes for FrtsCmd {}
>  
>  const NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS: u32 = 0x15;
>  const NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB: u32 = 0x19;
> @@ -149,24 +157,9 @@ impl FirmwareSignature<FwsecFirmware> for Bcrt30Rsa3kSignature {}
>  ///
>  /// Callers must ensure that the region of memory returned is not written for as long as the
>  /// returned reference is alive.
> -///
> -/// TODO[TRSM][COHA]: Remove this and `transmute_mut` once `CoherentAllocation::as_slice` is
> -/// available and we have a way to transmute objects implementing FromBytes, e.g.:
> -/// https://lore.kernel.org/lkml/20250330234039.29814-1-christiansantoslima21@gmail.com/
> -unsafe fn transmute<'a, 'b, T: Sized + FromBytes>(
> -    fw: &'a DmaObject,
> -    offset: usize,
> -) -> Result<&'b T> {
> -    if offset + size_of::<T>() > fw.size() {
> -        return Err(EINVAL);
> -    }
> -    if (fw.start_ptr() as usize + offset) % align_of::<T>() != 0 {
> -        return Err(EINVAL);
> -    }
> -
> -    // SAFETY: we have checked that the pointer is properly aligned that its pointed memory is
> -    // large enough the contains an instance of `T`, which implements `FromBytes`.
> -    Ok(unsafe { &*(fw.start_ptr().add(offset).cast::<T>()) })
> +unsafe fn transmute<T: Sized + FromBytes>(fw: &DmaObject, offset: usize) -> Result<&T> {
> +    // SAFETY: Guaranteed by the safety requirements of the function.

Please mention what is guaranteed and how it is guaranteed by the safety
requirements of the function (even if it seems trivial).

> +    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
> @@ -176,20 +169,12 @@ unsafe fn transmute<'a, 'b, T: Sized + FromBytes>(
>  ///
>  /// Callers must ensure that the region of memory returned is not read or written for as long as
>  /// the returned reference is alive.
> -unsafe fn transmute_mut<'a, 'b, T: Sized + FromBytes>(
> -    fw: &'a mut DmaObject,
> +unsafe fn transmute_mut<T: Sized + FromBytes + AsBytes>(
> +    fw: &mut DmaObject,
>      offset: usize,
> -) -> Result<&'b mut T> {
> -    if offset + size_of::<T>() > fw.size() {
> -        return Err(EINVAL);
> -    }
> -    if (fw.start_ptr_mut() as usize + offset) % align_of::<T>() != 0 {
> -        return Err(EINVAL);
> -    }
> -
> -    // SAFETY: we have checked that the pointer is properly aligned that its pointed memory is
> -    // large enough the contains an instance of `T`, which implements `FromBytes`.
> -    Ok(unsafe { &mut *(fw.start_ptr_mut().add(offset).cast::<T>()) })
> +) -> Result<&mut T> {
> +    // SAFETY: Guaranteed by the safety requirements of the function.

Same here.

> +    T::from_bytes_mut(unsafe { fw.as_slice_mut(offset, size_of::<T>())? }).ok_or(EINVAL)
>  }

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Daniel del Castillo]

Hi Danilo,

On 10/15/25 22:04, Danilo Krummrich wrote:
> Hi Daniel,
> 
> On 10/15/25 9:49 PM, Daniel del Castillo wrote:
>> This patch solves the existing mentions of COHA, a task
>> in the Nova task list about improving the `CoherentAllocation` API.
>> I confirmed by talking to Alexandre Courbot, that the reading/writing
>> methods in `CoherentAllocation` can never be safe, so
>> this patch doesn't actually change `CoherentAllocation`, but rather
>> tries to solve the existing references to [COHA].
> 
> This patch seems to address two different TODOs. Please split up the patch
> accordingly.

Sorry, I thought it was okay as they were part of the same task. Will do.

Thanks for the other comments as well, I'll apply them and send a new
version soon.

Cheers,
Daniel

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Danilo Krummrich]

On 10/16/25 11:13 PM, Daniel del Castillo wrote:
> Sorry, I thought it was okay as they were part of the same task. Will do.

No worries, no need to apologize -- thanks for your contribution!
> Thanks for the other comments as well, I'll apply them and send a new
> version soon.

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Daniel del Castillo]

Hi Danilo,

On 10/15/25 22:04, Danilo Krummrich wrote:
>> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
>> index 94f44bcfd748..639a99cf72c4 100644
>> --- a/drivers/gpu/nova-core/dma.rs
>> +++ b/drivers/gpu/nova-core/dma.rs
>> @@ -25,21 +25,11 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
>>      }
>>  
>>      pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
>> -        Self::new(dev, data.len()).map(|mut dma_obj| {
>> -            // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
>> -            // SAFETY:
>> -            // - `dma_obj`'s size is at least `data.len()`.
>> -            // - We have just created this object and there is no other user at this stage.
>> -            unsafe {
>> -                core::ptr::copy_nonoverlapping(
>> -                    data.as_ptr(),
>> -                    dma_obj.dma.start_ptr_mut(),
>> -                    data.len(),
>> -                );
>> -            }
>> -
>> -            dma_obj
>> -        })
>> +        let mut dma_obj = Self::new(dev, data.len())?;
>> +        // SAFETY: We have just created this object and there is no other user at this stage.
> 
> The safety comment should rather confirm that it is guaranteed that the device
> won't access this memory concurrently.

I actually don't know how this is guaranteed. It wasn't explicitly
explained before here, although unless I'm mistaken it was already a
requirement. Could you help me? I guess it's related to the already
mentioned fact that we just allocated this DMA memory and the device
isn't yet initialized?

Thanks,
Daniel

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Alexandre Courbot]

On Thu Oct 16, 2025 at 4:49 AM JST, Daniel del Castillo wrote:
> This patch solves the existing mentions of COHA, a task
> in the Nova task list about improving the `CoherentAllocation` API.
> I confirmed by talking to Alexandre Courbot, that the reading/writing
> methods in `CoherentAllocation` can never be safe, so
> this patch doesn't actually change `CoherentAllocation`, but rather
> tries to solve the existing references to [COHA].

This mention of background discussions should be in the comment part of
your commit (below the `---`).

>
> Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@gmail.com>
> ---
>  drivers/gpu/nova-core/dma.rs            |  20 ++---
>  drivers/gpu/nova-core/firmware/fwsec.rs | 104 ++++++++++--------------
>  2 files changed, 50 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
> index 94f44bcfd748..639a99cf72c4 100644
> --- a/drivers/gpu/nova-core/dma.rs
> +++ b/drivers/gpu/nova-core/dma.rs
> @@ -25,21 +25,11 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
>      }
>  
>      pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
> -        Self::new(dev, data.len()).map(|mut dma_obj| {
> -            // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
> -            // SAFETY:
> -            // - `dma_obj`'s size is at least `data.len()`.
> -            // - We have just created this object and there is no other user at this stage.
> -            unsafe {
> -                core::ptr::copy_nonoverlapping(
> -                    data.as_ptr(),
> -                    dma_obj.dma.start_ptr_mut(),
> -                    data.len(),
> -                );
> -            }
> -
> -            dma_obj
> -        })
> +        let mut dma_obj = Self::new(dev, data.len())?;
> +        // SAFETY: We have just created this object and there is no other user at this stage.
> +        unsafe { dma_obj.write(data, 0)? };
> +
> +        Ok(dma_obj)

Can you preserve the use of `map`? This removes the need for the
temporary variable.

<snip>
>  /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP falcon.
> @@ -260,32 +245,38 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>  
>          // Find the DMEM mapper section in the firmware.
>          for i in 0..hdr.entry_count as usize {
> -            let app: &FalconAppifV1 =
>              // SAFETY: we have exclusive access to `dma_object`.
> -            unsafe {
> +            let app: &FalconAppifV1 = unsafe {
>                  transmute(
>                      &dma_object,
> -                    hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize
> +                    hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize,
>                  )
>              }?;
>  
>              if app.id != NVFW_FALCON_APPIF_ID_DMEMMAPPER {
>                  continue;
>              }
> +            let dmem_base = app.dmem_base;
>  
>              // SAFETY: we have exclusive access to `dma_object`.
>              let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
> -                transmute_mut(
> -                    &mut dma_object,
> -                    (desc.imem_load_size + app.dmem_base) as usize,
> -                )
> +                transmute_mut(&mut dma_object, (desc.imem_load_size + dmem_base) as usize)
>              }?;
>  
> +            dmem_mapper.init_cmd = match cmd {
> +                FwsecCommand::Frts {
> +                    frts_addr: _,
> +                    frts_size: _,

Can the `{ .. }` syntax be used here?

> +                } => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS,
> +                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
> +            };
> +            let cmd_in_buffer_offset = dmem_mapper.cmd_in_buffer_offset;
> +
>              // SAFETY: we have exclusive access to `dma_object`.
>              let frts_cmd: &mut FrtsCmd = unsafe {
>                  transmute_mut(
>                      &mut dma_object,
> -                    (desc.imem_load_size + dmem_mapper.cmd_in_buffer_offset) as usize,
> +                    (desc.imem_load_size + cmd_in_buffer_offset) as usize,
>                  )
>              }?;
>  
> @@ -296,24 +287,19 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>                  size: 0,
>                  flags: 2,
>              };
> -
> -            dmem_mapper.init_cmd = match cmd {
> -                FwsecCommand::Frts {
> -                    frts_addr,
> -                    frts_size,
> -                } => {
> -                    frts_cmd.frts_region = FrtsRegion {
> -                        ver: 1,
> -                        hdr: size_of::<FrtsRegion>() as u32,
> -                        addr: (frts_addr >> 12) as u32,
> -                        size: (frts_size >> 12) as u32,
> -                        ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
> -                    };
> -
> -                    NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS
> -                }
> -                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
> -            };
> +            if let FwsecCommand::Frts {
> +                frts_addr,
> +                frts_size,
> +            } = cmd
> +            {
> +                frts_cmd.frts_region = FrtsRegion {
> +                    ver: 1,
> +                    hdr: size_of::<FrtsRegion>() as u32,
> +                    addr: (frts_addr >> 12) as u32,
> +                    size: (frts_size >> 12) as u32,
> +                    ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
> +                };
> +            }

I liked that the original code updated both `init_cmd` and `frts_region`
in the same match block. I understand it might be difficult to preserve
due to the borrowing rules, but can you try to preserve it if that's
possible at all?

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Daniel del Castillo]

Hi Alexandre,

Thanks for your comments!



On 10/17/25 03:36, Alexandre Courbot wrote:
> On Thu Oct 16, 2025 at 4:49 AM JST, Daniel del Castillo wrote:
>> This patch solves the existing mentions of COHA, a task
>> in the Nova task list about improving the `CoherentAllocation` API.
>> I confirmed by talking to Alexandre Courbot, that the reading/writing
>> methods in `CoherentAllocation` can never be safe, so
>> this patch doesn't actually change `CoherentAllocation`, but rather
>> tries to solve the existing references to [COHA].
> 
> This mention of background discussions should be in the comment part of
> your commit (below the `---`).

Noted, will do for the next version of the patch.

>>
>> Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@gmail.com>
>> ---
>>  drivers/gpu/nova-core/dma.rs            |  20 ++---
>>  drivers/gpu/nova-core/firmware/fwsec.rs | 104 ++++++++++--------------
>>  2 files changed, 50 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
>> index 94f44bcfd748..639a99cf72c4 100644
>> --- a/drivers/gpu/nova-core/dma.rs
>> +++ b/drivers/gpu/nova-core/dma.rs
>> @@ -25,21 +25,11 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
>>      }
>>  
>>      pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
>> -        Self::new(dev, data.len()).map(|mut dma_obj| {
>> -            // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
>> -            // SAFETY:
>> -            // - `dma_obj`'s size is at least `data.len()`.
>> -            // - We have just created this object and there is no other user at this stage.
>> -            unsafe {
>> -                core::ptr::copy_nonoverlapping(
>> -                    data.as_ptr(),
>> -                    dma_obj.dma.start_ptr_mut(),
>> -                    data.len(),
>> -                );
>> -            }
>> -
>> -            dma_obj
>> -        })
>> +        let mut dma_obj = Self::new(dev, data.len())?;
>> +        // SAFETY: We have just created this object and there is no other user at this stage.
>> +        unsafe { dma_obj.write(data, 0)? };
>> +
>> +        Ok(dma_obj)
> 
> Can you preserve the use of `map`? This removes the need for the
> temporary variable.
> 

Sure.> <snip>
>>  /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP falcon.
>> @@ -260,32 +245,38 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>  
>>          // Find the DMEM mapper section in the firmware.
>>          for i in 0..hdr.entry_count as usize {
>> -            let app: &FalconAppifV1 =
>>              // SAFETY: we have exclusive access to `dma_object`.
>> -            unsafe {
>> +            let app: &FalconAppifV1 = unsafe {
>>                  transmute(
>>                      &dma_object,
>> -                    hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize
>> +                    hdr_offset + hdr.header_size as usize + i * hdr.entry_size as usize,
>>                  )
>>              }?;
>>  
>>              if app.id != NVFW_FALCON_APPIF_ID_DMEMMAPPER {
>>                  continue;
>>              }
>> +            let dmem_base = app.dmem_base;
>>  
>>              // SAFETY: we have exclusive access to `dma_object`.
>>              let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
>> -                transmute_mut(
>> -                    &mut dma_object,
>> -                    (desc.imem_load_size + app.dmem_base) as usize,
>> -                )
>> +                transmute_mut(&mut dma_object, (desc.imem_load_size + dmem_base) as usize)
>>              }?;
>>  
>> +            dmem_mapper.init_cmd = match cmd {
>> +                FwsecCommand::Frts {
>> +                    frts_addr: _,
>> +                    frts_size: _,
> 
> Can the `{ .. }` syntax be used here?
> 
Yes! I didn't remember that syntax.


>> +                } => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS,
>> +                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
>> +            };
>> +            let cmd_in_buffer_offset = dmem_mapper.cmd_in_buffer_offset;
>> +
>>              // SAFETY: we have exclusive access to `dma_object`.
>>              let frts_cmd: &mut FrtsCmd = unsafe {
>>                  transmute_mut(
>>                      &mut dma_object,
>> -                    (desc.imem_load_size + dmem_mapper.cmd_in_buffer_offset) as usize,
>> +                    (desc.imem_load_size + cmd_in_buffer_offset) as usize,
>>                  )
>>              }?;
>>  
>> @@ -296,24 +287,19 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>                  size: 0,
>>                  flags: 2,
>>              };
>> -
>> -            dmem_mapper.init_cmd = match cmd {
>> -                FwsecCommand::Frts {
>> -                    frts_addr,
>> -                    frts_size,
>> -                } => {
>> -                    frts_cmd.frts_region = FrtsRegion {
>> -                        ver: 1,
>> -                        hdr: size_of::<FrtsRegion>() as u32,
>> -                        addr: (frts_addr >> 12) as u32,
>> -                        size: (frts_size >> 12) as u32,
>> -                        ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
>> -                    };
>> -
>> -                    NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS
>> -                }
>> -                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
>> -            };
>> +            if let FwsecCommand::Frts {
>> +                frts_addr,
>> +                frts_size,
>> +            } = cmd
>> +            {
>> +                frts_cmd.frts_region = FrtsRegion {
>> +                    ver: 1,
>> +                    hdr: size_of::<FrtsRegion>() as u32,
>> +                    addr: (frts_addr >> 12) as u32,
>> +                    size: (frts_size >> 12) as u32,
>> +                    ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
>> +                };
>> +            }
> 
> I liked that the original code updated both `init_cmd` and `frts_region`
> in the same match block. I understand it might be difficult to preserve
> due to the borrowing rules, but can you try to preserve it if that's
> possible at all?

I agree it was nicer. I tried to preserve it, but I don't see a way to
do it cleanly, as I can't keep both mutable references at the same time.
What I could do is only check `cmd` once, set `init_cmd` and store an
`Option<FrtsRegion>` that I will later use to set `frts_region` if it's
not `None`. Let me know if you prefer that.

Cheers,
Daniel

Re: [PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]

[Alexandre Courbot]

On Sun Oct 19, 2025 at 8:57 PM JST, Daniel del Castillo wrote:
>>> @@ -296,24 +287,19 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>>                  size: 0,
>>>                  flags: 2,
>>>              };
>>> -
>>> -            dmem_mapper.init_cmd = match cmd {
>>> -                FwsecCommand::Frts {
>>> -                    frts_addr,
>>> -                    frts_size,
>>> -                } => {
>>> -                    frts_cmd.frts_region = FrtsRegion {
>>> -                        ver: 1,
>>> -                        hdr: size_of::<FrtsRegion>() as u32,
>>> -                        addr: (frts_addr >> 12) as u32,
>>> -                        size: (frts_size >> 12) as u32,
>>> -                        ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
>>> -                    };
>>> -
>>> -                    NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS
>>> -                }
>>> -                FwsecCommand::Sb => NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB,
>>> -            };
>>> +            if let FwsecCommand::Frts {
>>> +                frts_addr,
>>> +                frts_size,
>>> +            } = cmd
>>> +            {
>>> +                frts_cmd.frts_region = FrtsRegion {
>>> +                    ver: 1,
>>> +                    hdr: size_of::<FrtsRegion>() as u32,
>>> +                    addr: (frts_addr >> 12) as u32,
>>> +                    size: (frts_size >> 12) as u32,
>>> +                    ftype: NVFW_FRTS_CMD_REGION_TYPE_FB,
>>> +                };
>>> +            }
>> 
>> I liked that the original code updated both `init_cmd` and `frts_region`
>> in the same match block. I understand it might be difficult to preserve
>> due to the borrowing rules, but can you try to preserve it if that's
>> possible at all?
>
> I agree it was nicer. I tried to preserve it, but I don't see a way to
> do it cleanly, as I can't keep both mutable references at the same time.
> What I could do is only check `cmd` once, set `init_cmd` and store an
> `Option<FrtsRegion>` that I will later use to set `frts_region` if it's
> not `None`. Let me know if you prefer that.

Yeah, I agree the borrow checker will get in the way now that we have
fixed the lifetimes.

What I wanted to avoid is performing the same match operation on `cmd`
twice, but if that's difficult then I guess we can live with it. Using a
temporary `Option` comes down to the same in the end, except that the
second test is indirect.