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

Joel Fernandes
2025-05-01T13:52:39Z

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.

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

(Also it is a bit weird that the namespace for chipsets for > GA10x is
ga102::GA102::). Example, Chipset::GA104 uses the HAL in Ga102).

thanks,

- Joel