On Wed Jun 4, 2025 at 6:32 AM JST, Lyude Paul wrote:
<snip>
>> +unsafe fn transmute<'a, 'b, T: Sized + FromBytes>(
>> + fw: &'a DmaObject,
>> + offset: usize,
>> +) -> Result<&'b T> {
>> + if offset + core::mem::size_of::<T>() > fw.size() {
>> + return Err(EINVAL);
>> + }
>> + if (fw.start_ptr() as usize + offset) % core::mem::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) as *const T) })
>
> Why not .cast()?
No reason - fixed, thanks!
>
>> +}
>> +
>> +/// Reinterpret the area starting from `offset` in `fw` as a mutable
>> instance of `T` (which must
>> +/// implement [`FromBytes`]) and return a reference to it.
>> +///
>> +/// # 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,
>> + offset: usize,
>> +) -> Result<&'b mut T> {
>> + if offset + core::mem::size_of::<T>() > fw.size() {
>> + return Err(EINVAL);
>> + }
>> + if (fw.start_ptr_mut() as usize + offset) % core::mem::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) as *mut T) })
>> +}
>> +
>> +impl FirmwareDmaObject<FwsecFirmware> {
>> + /// Patch the Fwsec firmware image in `fw` to run the command `cmd`.
>> + fn patch_command(&mut self, v3_desc: &FalconUCodeDescV3, cmd:
>> FwsecCommand) -> Result<()> {
>> + let hdr_offset = (v3_desc.imem_load_size +
>> v3_desc.interface_offset) as usize;
>> + // SAFETY: we have an exclusive reference to `self`, and no caller
>> should have shared
>> + // `self` with the hardware yet.
>> + let hdr: &FalconAppifHdrV1 = unsafe { transmute(&self.0,
>> hdr_offset) }?;
>> +
>> + if hdr.version != 1 {
>> + return Err(EINVAL);
>> + }
>> +
>> + // Find the DMEM mapper section in the firmware.
>> + for i in 0..hdr.entry_count as usize {
>> + let app: &FalconAppifV1 =
>> + // SAFETY: we have an exclusive reference to `self`, and no
>> caller should have shared
>> + // `self` with the hardware yet.
>> + unsafe {
>> + transmute(
>> + &self.0,
>> + hdr_offset + hdr.header_size as usize + i *
>> hdr.entry_size as usize
>> + )
>> + }?;
>> +
>> + if app.id != NVFW_FALCON_APPIF_ID_DMEMMAPPER {
>> + continue;
>> + }
>> +
>> + // SAFETY: we have an exclusive reference to `self`, and no
>> caller should have shared
>> + // `self` with the hardware yet.
>> + let dmem_mapper: &mut FalconAppifDmemmapperV3 = unsafe {
>> + transmute_mut(
>> + &mut self.0,
>> + (v3_desc.imem_load_size + app.dmem_base) as usize,
>> + )
>> + }?;
>> +
>> + // SAFETY: we have an exclusive reference to `self`, and no
>> caller should have shared
>> + // `self` with the hardware yet.
>> + let frts_cmd: &mut FrtsCmd = unsafe {
>> + transmute_mut(
>> + &mut self.0,
>> + (v3_desc.imem_load_size +
>> dmem_mapper.cmd_in_buffer_offset) as usize,
>> + )
>> + }?;
>> +
>> + frts_cmd.read_vbios = ReadVbios {
>> + ver: 1,
>> + hdr: core::mem::size_of::<ReadVbios>() as u32,
>
> I think if we're using size_of and align_of this many times it would be worth
> just importing it
Indeed, especially since they seem to already be imported by the kernel
prelude.
>
>> + addr: 0,
>> + size: 0,
>> + flags: 2,
>> + };
>> +
>> + dmem_mapper.init_cmd = match cmd {
>> + FwsecCommand::Frts {
>> + frts_addr,
>> + frts_size,
>> + } => {
>> + frts_cmd.frts_region = FrtsRegion {
>> + ver: 1,
>> + hdr: core::mem::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,
>> + };
>> +
>> + // Return early as we found and patched the DMEMMAPPER region.
>> + return Ok(());
>> + }
>> +
>> + Err(ENOTSUPP)
>> + }
>> +}
>> +
>> +/// The FWSEC microcode, extracted from the BIOS and to be run on the GSP
>> falcon.
>> +///
>> +/// It is responsible for e.g. carving out the WPR2 region as the first
>> step of the GSP bootflow.
>> +pub(crate) struct FwsecFirmware {
>> + desc: FalconUCodeDescV3,
>> + ucode: FirmwareDmaObject<Self>,
>> +}
>> +
>> +impl FalconLoadParams for FwsecFirmware {
>> + fn imem_load_params(&self) -> FalconLoadTarget {
>> + FalconLoadTarget {
>> + src_start: 0,
>> + dst_start: self.desc.imem_phys_base,
>> + len: self.desc.imem_load_size,
>> + }
>> + }
>> +
>> + fn dmem_load_params(&self) -> FalconLoadTarget {
>> + FalconLoadTarget {
>> + src_start: self.desc.imem_load_size,
>> + dst_start: self.desc.dmem_phys_base,
>> + len: Layout::from_size_align(self.desc.dmem_load_size as usize,
>> 256)
>> + // Cannot panic, as 256 is non-zero and a power of 2.
>> + .unwrap()
>
> Why not just unwrap_unchecked() then? Or do we still want a possible panic
> here just to make sure we didn't make a mistake?
`unwrap_unchecked` requires an `unsafe` block, which I think it not
really worth here. I'd expect the compiler to optimize the `unwrap` out
anyway.
