Message-Id: <DADCKUVFXPLH.2DWHTBQ76T3PD@nvidia.com>
Date: Wed, 04 Jun 2025 10:11:41 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Lyude Paul" <lyude@...hat.com>, "Miguel Ojeda" <ojeda@...nel.org>,
 "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>,
 "Gary Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Danilo
 Krummrich" <dakr@...nel.org>, "David Airlie" <airlied@...il.com>, "Simona
 Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>
Cc: "John Hubbard" <jhubbard@...dia.com>, "Ben Skeggs" <bskeggs@...dia.com>,
 "Joel Fernandes" <joelagnelf@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 "Alistair Popple" <apopple@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <nouveau@...ts.freedesktop.org>,
 <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v4 19/20] gpu: nova-core: extract FWSEC from BIOS and
 patch it to run FWSEC-FRTS

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.

Powered by blists - more mailing lists