Message-ID: <f18a9db5-d2e4-4f14-bfa1-dd6542d3d733@nvidia.com>
Date: Thu, 1 May 2025 10:41:12 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
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>, 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>,
 Jonathan Corbet <corbet@....net>, John Hubbard <jhubbard@...dia.com>,
 Ben Skeggs <bskeggs@...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 v2 15/21] gpu: nova-core: add falcon register definitions
 and base code

On 5/1/2025 10:18 AM, Alexandre Courbot wrote:
[..]
>> On Thu, May 01, 2025 at 09:58:33PM +0900, Alexandre Courbot wrote:
>>> Add the common Falcon code and HAL for Ampere GPUs, and instantiate the
>>> GSP and SEC2 Falcons that will be required to boot the GSP.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>>> ---
>>>  drivers/gpu/nova-core/falcon.rs           | 546 ++++++++++++++++++++++++++++++
>>>  drivers/gpu/nova-core/falcon/gsp.rs       |  25 ++
>>>  drivers/gpu/nova-core/falcon/hal.rs       |  55 +++
>>>  drivers/gpu/nova-core/falcon/hal/ga102.rs | 115 +++++++
>>>  drivers/gpu/nova-core/falcon/sec2.rs      |   8 +
>>>  drivers/gpu/nova-core/gpu.rs              |  11 +
>>>  drivers/gpu/nova-core/nova_core.rs        |   1 +
>>>  drivers/gpu/nova-core/regs.rs             | 125 +++++++
>>>  drivers/gpu/nova-core/util.rs             |   1 -
>>>  9 files changed, 886 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..7cae45645e548bab5b85cb53880898cedbae778a
>>> --- /dev/null
>>> +++ b/drivers/gpu/nova-core/falcon.rs
>>> @@ -0,0 +1,546 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Falcon microprocessor base support
>>> +
>>> +// To be removed when all code is used.
>>> +#![expect(dead_code)]
>>> +
>>> +use core::time::Duration;
>>> +use hal::FalconHal;
>>> +use kernel::bindings;
>>> +use kernel::device;
>>> +use kernel::devres::Devres;
>>> +use kernel::prelude::*;
>>> +use kernel::sync::Arc;
>>> +
>>> +use crate::driver::Bar0;
>>> +use crate::gpu::Chipset;
>>> +use crate::regs;
>>> +use crate::util;
>>> +
>>> +pub(crate) mod gsp;
>>> +mod hal;
>>> +pub(crate) mod sec2;
>>> +
>>> +/// Revision number of a falcon core, used in the [`crate::regs::NV_PFALCON_FALCON_HWCFG1`]
>>> +/// register.
>>> +#[repr(u8)]
>>> +#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
>>> +pub(crate) enum FalconCoreRev {
>>> +    #[default]
>>> +    Rev1 = 1,
>>> +    Rev2 = 2,
>>> +    Rev3 = 3,
>>> +    Rev4 = 4,
>>> +    Rev5 = 5,
>>> +    Rev6 = 6,
>>> +    Rev7 = 7,
>>> +}
>>> +
>>> +impl TryFrom<u8> for FalconCoreRev {
>>> +    type Error = Error;
>>> +
>>> +    fn try_from(value: u8) -> core::result::Result<Self, Self::Error> {
>>> +        use FalconCoreRev::*;
>>> +
>>> +        let rev = match value {
>>> +            1 => Rev1,
>>> +            2 => Rev2,
>>> +            3 => Rev3,
>>> +            4 => Rev4,
>>> +            5 => Rev5,
>>> +            6 => Rev6,
>>> +            7 => Rev7,
>>> +            _ => return Err(EINVAL),
>>> +        };
>>> +
>>> +        Ok(rev)
>>> +    }
>>> +}
>>> +
>>> +/// Revision subversion number of a falcon core, used in the
>>> +/// [`crate::regs::NV_PFALCON_FALCON_HWCFG1`] register.
>>> +#[repr(u8)]
>>> +#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
>>> +pub(crate) enum FalconCoreRevSubversion {
>>> +    #[default]
>>> +    Subversion0 = 0,
>>> +    Subversion1 = 1,
>>> +    Subversion2 = 2,
>>> +    Subversion3 = 3,
>>> +}
>>> +
>>> +impl TryFrom<u8> for FalconCoreRevSubversion {
>>> +    type Error = Error;
>>> +
>>> +    fn try_from(value: u8) -> Result<Self> {
>>> +        use FalconCoreRevSubversion::*;
>>> +
>>> +        let sub_version = match value & 0b11 {
>>> +            0 => Subversion0,
>>> +            1 => Subversion1,
>>> +            2 => Subversion2,
>>> +            3 => Subversion3,
>>> +            _ => return Err(EINVAL),
>>> +        };
>>> +
>>> +        Ok(sub_version)
>>> +    }
>>> +}
>>> +
>>> +/// Security model of a falcon core, used in the [`crate::regs::NV_PFALCON_FALCON_HWCFG1`]
>>> +/// register.
>>> +#[repr(u8)]
>>> +#[derive(Debug, Default, Copy, Clone)]
>>> +pub(crate) enum FalconSecurityModel {
>>> +    /// Non-Secure: runs unsigned code without privileges.
>>> +    #[default]
>>> +    None = 0,
>>> +    /// Low-secure: runs unsigned code with some privileges. Can only be entered from `Heavy` mode.
>>
>> This is not true. Low-secure is also (has to be) signed and the signatures
>> are verified by High-secure code. I can/will go fix that up in my follow-up doc
>> patches.
> 
> True, but contrary to HS code, the signature in the LS code is not a
> hardware (or rather boot ROM) requirement - it's just that the HS code
> decides to implement this policy and you could very well have a HS
> loader that loads some code and switches to LS without further
> verification. The point being that you cannot enter LS mode directly and
> need to go through a HS loader first.
> 
> Nonetheless, you are right that in practice the HS code will not switch
> to LS without due verification, and my use of "unsigned" is confusing.
> Let me reword this.

Thanks, I wonder if there is any current example of such unsigned LS code. IIUC,
all the LS code is either coming from either the VBIOS or the firmware binaries,
both of which can be modified/re-flashed. Since LS still has some privileges, it
means that it is a bit of security issue. I think you're right though, in theory
LS can be run unverified but I'd think it is atypical.

>>
>>> +/// Returns a boxed falcon HAL adequate for the passed `chipset`.
>>> +///
>>> +/// We use this function and a heap-allocated trait object instead of statically defined trait
>>> +/// objects because of the two-dimensional (Chipset, Engine) lookup required to return the
>>> +/// requested HAL.
>>> +///
>>> +/// TODO: replace the return type with `KBox` once it gains the ability to host trait objects.
>>> +pub(crate) fn create_falcon_hal<E: FalconEngine + 'static>(
>>> +    chipset: Chipset,
>>> +) -> Result<Arc<dyn FalconHal<E>>> {
>>> +    let hal = match chipset {
>>> +        Chipset::GA102 | Chipset::GA103 | Chipset::GA104 | Chipset::GA106 | Chipset::GA107 => {
>>> +            Arc::new(ga102::Ga102::<E>::new(), GFP_KERNEL)? as Arc<dyn FalconHal<E>>
>>
>> I am guessing macro-fication of this did not pan out? i.e. I think we
>> discussed:
>> 1. Seeing if we can reduce/get rid of Arc in favor of static allocation.
>> 2. Simplify the chain of GAxx | GAyy..
>> But nothing that cannot be done as a follow-up improvement..
> 
> Yeah, my macro-fu is still lacking it seems. ^_^;

:-D. It may or may not be worth complicating it though, but I was considering in
the future, to minimize the number of places that need to be modified (and thus
more possible room for errors) for new chipset additions.

>> (Also it is a bit weird that the namespace for chipsets for > GA10x is
>> ga102::GA102::). Example, Chipset::GA104 uses the HAL in Ga102).
> 
> It is a convention since Nouveau (but I believe OpenRM as well?) to name
> a HAL after the chipset with the lowest number when subsequent chipsets
> can also use it.

Ah ok! Maybe worth a documentation comment somewhere as well? Or maybe not. ;-)

thanks,

 - Joel

Powered by blists - more mailing lists