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 <[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. + 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/[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. + 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
