Re: [PATCH v3 18/19] gpu: nova-core: extract FWSEC from BIOS and patch it to run FWSEC-FRTS

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu May 15, 2025 at 1:38 AM JST, Danilo Krummrich wrote:
> On Wed, May 07, 2025 at 10:52:45PM +0900, Alexandre Courbot wrote:
>> The FWSEC firmware needs to be extracted from the VBIOS and patched with
>> the desired command, as well as the right signature. Do this so we are
>> ready to load and run this firmware into the GSP falcon and create the
>> FRTS region.
>> 
>> [joelagnelf@nvidia.com: give better names to FalconAppifHdrV1's fields]
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/dma.rs            |   3 -
>>  drivers/gpu/nova-core/firmware.rs       |  18 ++
>>  drivers/gpu/nova-core/firmware/fwsec.rs | 359 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/nova-core/gpu.rs            |  20 +-
>>  drivers/gpu/nova-core/vbios.rs          |   3 -
>>  5 files changed, 395 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
>> index 9d90ae01d0044eaab4ddbc3eba216741d7a623ef..a12d0dff574aa38fb5eb8f4d759611af2f8ba3ec 100644
>> --- a/drivers/gpu/nova-core/dma.rs
>> +++ b/drivers/gpu/nova-core/dma.rs
>> @@ -2,9 +2,6 @@
>>  
>>  //! Simple DMA object wrapper.
>>  
>> -// To be removed when all code is used.
>> -#![expect(dead_code)]
>> -
>>  use core::ops::{Deref, DerefMut};
>>  
>>  use kernel::device;
>> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> index 960982174d834c7c66a47ecfb3a15bf47116b2c5..3945fd18499555ddd6fb2e0ea69535b40fcc4b08 100644
>> --- a/drivers/gpu/nova-core/firmware.rs
>> +++ b/drivers/gpu/nova-core/firmware.rs
>> @@ -8,9 +8,12 @@
>>  use kernel::prelude::*;
>>  use kernel::str::CString;
>>  
>> +use crate::dma::DmaObject;
>>  use crate::gpu;
>>  use crate::gpu::Chipset;
>>  
>> +pub(crate) mod fwsec;
>> +
>>  pub(crate) const FIRMWARE_VERSION: &str = "535.113.01";
>>  
>>  /// Structure encapsulating the firmware blobs required for the GPU to operate.
>> @@ -86,6 +89,21 @@ pub(crate) fn size(&self) -> usize {
>>      }
>>  }
>>  
>> +/// Patch the `ucode_dma` firmware at offset `sig_base_img` with `signature`.
>> +fn patch_signature(ucode_dma: &mut DmaObject, signature: &[u8], sig_base_img: usize) -> Result<()> {
>> +    if sig_base_img + signature.len() > ucode_dma.size() {
>> +        return Err(EINVAL);
>> +    }
>> +
>> +    // SAFETY: we are the only user of this object, so there cannot be any race.
>> +    let dst = unsafe { ucode_dma.start_ptr_mut().add(sig_base_img) };
>> +
>> +    // SAFETY: `signature` and `dst` are valid, properly aligned, and do not overlap.
>> +    unsafe { core::ptr::copy_nonoverlapping(signature.as_ptr(), dst, signature.len()) };
>> +
>> +    Ok(())
>> +}
>
> Why is this not in firmware/fwsec.rs, like patch_command()?

Ah, there is no way to know it now, but this function will also be used
to patch the booter firmware that runs on sec2, so having it here makes
it available to both sub-modules. I'm fine with moving it into the fwsec
module temporarily if you prefer though.

>
> Also, please wrap the ucode DmaObject in its own type, i.e.
> `struct UcodeDma(DmaObject)` and make the patch_*() functions methods of this
> type. They're only applicable for the ucode DmaObject.

Indeed, good idea. We will event want to specialize that type against
the kind of firmware as not all patching methods may be applicable
depending on the firmware.

<snip>
>> +impl FwsecFirmware {
>> +    /// Extract the Fwsec firmware from `bios` and patch it to run with the `cmd` command.
>> +    pub(crate) fn new(
>> +        falcon: &Falcon<Gsp>,
>> +        dev: &Device<device::Bound>,
>> +        bar: &Bar0,
>> +        bios: &Vbios,
>> +        cmd: FwsecCommand,
>> +    ) -> Result<Self> {
>> +        let v3_desc = bios.fwsec_header(dev)?;
>> +        let ucode = bios.fwsec_ucode(dev)?;
>> +
>> +        let mut ucode_dma = DmaObject::from_data(dev, ucode)?;
>> +        patch_command(&mut ucode_dma, v3_desc, cmd)?;
>> +
>> +        const SIG_SIZE: usize = 96 * 4;
>
> 96 * 4? :-)

Mmmm let me look that up. ^_^; But I think it means that a signature is
made of 96 32-bit integers.

>
>> +        let signatures = bios.fwsec_sigs(dev)?;
>> +        let sig_base_img = (v3_desc.imem_load_size + v3_desc.pkc_data_offset) as usize;
>> +
>> +        if v3_desc.signature_count != 0 {
>> +            // Patch signature.
>> +            let desc_sig_versions = v3_desc.signature_versions as u32;
>> +            let reg_fuse_version = falcon.get_signature_reg_fuse_version(
>> +                bar,
>> +                v3_desc.engine_id_mask,
>> +                v3_desc.ucode_id,
>> +            )?;
>> +            dev_dbg!(
>> +                dev,
>> +                "desc_sig_versions: {:#x}, reg_fuse_version: {}\n",
>> +                desc_sig_versions,
>> +                reg_fuse_version
>> +            );
>> +            let signature_idx = {
>> +                let reg_fuse_version_bit = 1 << reg_fuse_version;
>> +
>> +                // Check if the fuse version is supported by the firmware.
>> +                if desc_sig_versions & reg_fuse_version_bit == 0 {
>> +                    dev_warn!(
>> +                        dev,
>> +                        "no matching signature: {:#x} {:#x}\n",
>> +                        reg_fuse_version_bit,
>> +                        v3_desc.signature_versions
>> +                    );
>
> Looks like this should be dev_err!().

Indeed, fixed.