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