Re: [PATCH v4 15/20] gpu: nova-core: firmware: add ucode descriptor used by FWSEC-FRTS

[Danilo Krummrich]

On Wed, May 21, 2025 at 03:45:10PM +0900, Alexandre Courbot wrote:
> FWSEC-FRTS is the first firmware we need to run on the GSP falcon in
> order to initiate the GSP boot process. Introduce the structure that
> describes it.
> 
> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/firmware.rs | 43 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 4b8a38358a4f6da2a4d57f8db50ea9e788c3e4b5..f675fb225607c3efd943393086123b7aeafd7d4f 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -41,6 +41,49 @@ pub(crate) fn new(dev: &device::Device, chipset: Chipset, ver: &str) -> Result<F
>      }
>  }
>  
> +/// Structure used to describe some firmwares, notably FWSEC-FRTS.
> +#[repr(C)]
> +#[derive(Debug, Clone)]
> +pub(crate) struct FalconUCodeDescV3 {
> +    /// Header defined by `NV_BIT_FALCON_UCODE_DESC_HEADER_VDESC*` in OpenRM.
> +    ///
> +    /// Bits `31:16` contain the size of the header, after which the actual ucode data starts.

The field is private; this information is much more needed in Self::size().

> +    hdr: u32,
> +    /// Stored size of the ucode after the header.
> +    stored_size: u32,
> +    /// Offset in `DMEM` at which the signature is expected to be found.
> +    pub(crate) pkc_data_offset: u32,
> +    /// Offset after the code segment at which the app headers are located.
> +    pub(crate) interface_offset: u32,
> +    /// Base address at which to load the code segment into `IMEM`.
> +    pub(crate) imem_phys_base: u32,
> +    /// Size in bytes of the code to copy into `IMEM`.
> +    pub(crate) imem_load_size: u32,
> +    /// Virtual `IMEM` address (i.e. `tag`) at which the code should start.
> +    pub(crate) imem_virt_base: u32,
> +    /// Base address at which to load the data segment into `DMEM`.
> +    pub(crate) dmem_phys_base: u32,
> +    /// Size in bytes of the data to copy into `DMEM`.
> +    pub(crate) dmem_load_size: u32,
> +    /// Mask of the falcon engines on which this firmware can run.
> +    pub(crate) engine_id_mask: u16,
> +    /// ID of the ucode used to infer a fuse register to validate the signature.
> +    pub(crate) ucode_id: u8,
> +    /// Number of signatures in this firmware.
> +    pub(crate) signature_count: u8,
> +    /// Versions of the signatures, used to infer a valid signature to use.
> +    pub(crate) signature_versions: u16,
> +    _reserved: u16,
> +}
> +
> +// To be removed once that code is used.
> +#[expect(dead_code)]
> +impl FalconUCodeDescV3 {

    const HDR_SIZE_SHIFT: u32 = 16;
    const HDR_SIZE_MASK: u32 = 0xffff0000;

> +    pub(crate) fn size(&self) -> usize {
> +        ((self.hdr & 0xffff0000) >> 16) as usize

	((self.hdr & HDR_SIZE_MASK) >> Self::HDR_SIZE_SHIFT)

In this case it may look a bit pointless, but I think it would make sense to
establish to store consts for shifts and masks in general, such that one can get
an easy overview of the layout of the structure.