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` V2 -> V3: Further splitting. Capitalization for existing comments has its own patch Fix typo. s/unitialized/uninitialized Rebase --- drivers/gpu/nova-core/firmware/fwsec.rs | 117 +++++++++++------------- 1 file changed, 54 insertions(+), 63 deletions(-) diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs index ce78c1563754..4f268fe09573 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: This struct doesn't contain uninitialized bytes and doesn't have interior mutability. +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: This struct doesn't contain uninitialized bytes and doesn't have interior mutability. +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: This struct doesn't contain uninitialized bytes and doesn't have interior mutability. +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: This struct doesn't contain uninitialized 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..usize::from(hdr.entry_count) { - let app: &FalconAppifV1 = // SAFETY: we have exclusive access to `dma_object`. - unsafe { + let app: &FalconAppifV1 = unsafe { transmute( &dma_object, - hdr_offset + usize::from(hdr.header_size) + i * usize::from(hdr.entry_size) + hdr_offset + usize::from(hdr.header_size) + i * usize::from(hdr.entry_size), ) }?; 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: u32::try_from(size_of::<FrtsRegion>())?, - addr: u32::try_from(frts_addr >> 12)?, - size: u32::try_from(frts_size >> 12)?, - 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: u32::try_from(size_of::<FrtsRegion>())?, + addr: u32::try_from(frts_addr >> 12)?, + size: u32::try_from(frts_size >> 12)?, + 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.2
