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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DADG4KHEFED5.1YMMF99ZG9YQQ@nvidia.com>
Date: Wed, 04 Jun 2025 12:58:29 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: "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>, "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>, "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 15/20] gpu: nova-core: firmware: add ucode descriptor
 used by FWSEC-FRTS

On Mon Jun 2, 2025 at 9:26 PM JST, Danilo Krummrich wrote:
> 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@...dia.com>
>> ---
>>  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().

Indeed.

>
>> +    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.

Not pointless at all, this is actually a good habit to take.

The (updated) register macro will also give us the ability to define
register-like field types without any I/O ops, which could be used in
such cases as well for more clarity. But that's for after this series.
:)