Re: [PATCH v2 15/21] gpu: nova-core: add falcon register definitions and base code

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

On Thu May 1, 2025 at 10:52 PM JST, Joel Fernandes wrote:
> Hello Alex,
>
> 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@nvidia.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.

>
>> +/// 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. ^_^;

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