Alexandre Courbot
2025-Oct-17 01:36 UTC
[PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]
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 at 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?
Daniel del Castillo
2025-Oct-19 11:57 UTC
[PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]
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 at 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
Alexandre Courbot
2025-Oct-22 13:35 UTC
[PATCH 1/2] nova-core: Solve mentions of `CoherentAllocation` improvements [COHA]
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.