This patch solves one of the existing mentions of COHA, a task in the Nova task list about improving the `CoherentAllocation` API. It uses the new `from_bytes` method from the `FromBytes` trait as well as the `as_slice` and `as_slice_mut` methods from `CoherentAllocation`.
Signed-off-by: Daniel del Castillo <[email protected]> --- 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 one of the existing references to [COHA]. V1 -> V2: Split previous patch into two. One per reference to COHA. Improved comments. Let me know if they are okay now. Use of `{...}` syntax for the `if let` drivers/gpu/nova-core/firmware/fwsec.rs | 129 +++++++++++------------- 1 file changed, 60 insertions(+), 69 deletions(-) diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs index 8edbb5c0572c..507ef3868565 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; @@ -35,7 +35,7 @@ struct FalconAppifHdrV1 { entry_size: u8, entry_count: u8, } -// SAFETY: any byte sequence is valid for this struct. +// SAFETY: Any byte sequence is valid for this struct. unsafe impl FromBytes for FalconAppifHdrV1 {} #[repr(C, packed)] @@ -44,7 +44,7 @@ struct FalconAppifV1 { id: u32, dmem_base: u32, } -// SAFETY: any byte sequence is valid for this struct. +// SAFETY: Any byte sequence is valid for this struct. unsafe impl FromBytes for FalconAppifV1 {} #[derive(Debug)] @@ -68,8 +68,10 @@ struct FalconAppifDmemmapperV3 { ucode_cmd_mask1: u32, multi_tgt_tbl: u32, } -// SAFETY: any byte sequence is valid for this struct. +// SAFETY: Any byte sequence is valid for this struct. unsafe impl FromBytes for FalconAppifDmemmapperV3 {} +// SAFETY: This struct doesn't contain unitialized bytes and doesn't have interior mutability. +unsafe impl AsBytes for FalconAppifDmemmapperV3 {} #[derive(Debug)] #[repr(C, packed)] @@ -80,8 +82,10 @@ struct ReadVbios { size: u32, flags: u32, } -// SAFETY: any byte sequence is valid for this struct. +// SAFETY: Any byte sequence is valid for this struct. unsafe impl FromBytes for ReadVbios {} +// SAFETY: This struct doesn't contain unitialized bytes and doesn't have interior mutability. +unsafe impl AsBytes for ReadVbios {} #[derive(Debug)] #[repr(C, packed)] @@ -92,8 +96,10 @@ struct FrtsRegion { size: u32, ftype: u32, } -// SAFETY: any byte sequence is valid for this struct. +// SAFETY: Any byte sequence is valid for this struct. unsafe impl FromBytes for FrtsRegion {} +// SAFETY: This struct doesn't contain unitialized bytes and doesn't have interior mutability. +unsafe impl AsBytes for FrtsRegion {} const NVFW_FRTS_CMD_REGION_TYPE_FB: u32 = 2; @@ -102,8 +108,10 @@ struct FrtsCmd { read_vbios: ReadVbios, frts_region: FrtsRegion, } -// SAFETY: any byte sequence is valid for this struct. +// SAFETY: Any byte sequence is valid for this struct. unsafe impl FromBytes for FrtsCmd {} +// SAFETY: This struct doesn't contain unitialized bytes and doesn't have interior mutability. +unsafe impl AsBytes for FrtsCmd {} const NVFW_FALCON_APPIF_DMEMMAPPER_CMD_FRTS: u32 = 0x15; const NVFW_FALCON_APPIF_DMEMMAPPER_CMD_SB: u32 = 0x19; @@ -147,26 +155,15 @@ impl FirmwareSignature<FwsecFirmware> for Bcrt30Rsa3kSignature {} /// /// # Safety /// -/// 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>()) }) +/// * Callers must ensure that the device does not read/write to/from memory while the returned +/// reference is live. +/// * Callers must ensure that this call does not race with a write to the same region while +/// the returned reference is live. +unsafe fn transmute<T: Sized + FromBytes>(fw: &DmaObject, offset: usize) -> Result<&T> { + // SAFETY: The safety requirements of the function guarantee the device won't read + // or write to memory while the reference is alive and that this call won't race + // with writes to the same memory region. + 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 @@ -174,22 +171,18 @@ unsafe fn transmute<'a, 'b, T: Sized + FromBytes>( /// /// # Safety /// -/// 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, +/// * Callers must ensure that the device does not read/write to/from memory while the returned +/// slice is live. +/// * Callers must ensure that this call does not race with a read or write to the same region +/// while the returned slice is live. +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: The safety requirements of the function guarantee the device won't read + // or write to memory while the reference is alive and that this call won't race + // with writes or reads to the same memory region. + 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 +253,35 @@ 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 { .. } => 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 +292,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.1
