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

[Daniel del Castillo <delcastillodelarosadaniel@gmail.com>]

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