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

["Alexandre Courbot" <acourbot@nvidia.com>]

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?