Re: [PATCH v3 8/8] gpu: nova-core: add NVKV GSP_INIT schemas

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

On Mon Sep 28, 2026 at 5:42 PM JST, Eliot Courtney wrote:
> Add the first user of NVKV encode/decode which is the request and
> response for GSP init. For now this is exercised via unit tests. Later
> patches will support GMCAPI in `Cmdq` and use these messages.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/fw/commands.rs | 447 ++++++++++++++++++++++++++++++-
>  drivers/gpu/nova-core/gsp/nvkv.rs        |   3 -
>  drivers/gpu/nova-core/gsp/nvkv/decode.rs |   1 +
>  drivers/gpu/nova-core/gsp/nvkv/encode.rs |   1 +
>  4 files changed, 448 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
> index 32856ff74183..02de225af917 100644
> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
> @@ -4,6 +4,8 @@
>  use core::ops::Range;
>  
>  use kernel::{
> +    alloc::ArrayVec,
> +    bitfield,
>      device,
>      pci,
>      prelude::*,
> @@ -15,7 +17,21 @@
>  
>  use crate::{
>      gpu::Chipset,
> -    gsp::GSP_PAGE_SIZE,
> +    gsp::{
> +        nvkv::{
> +            nvkv_decode,
> +            nvkv_encode,
> +            Accumulated,
> +            Array,
> +            DecoderValue,
> +            Encodable,
> +            Encoder,
> +            Key,
> +            KeyId,
> +            Required, //
> +        },
> +        GSP_PAGE_SIZE, //
> +    },
>      num::IntoSafeCast, //
>  };
>  
> @@ -230,3 +246,432 @@ unsafe impl AsBytes for UnloadingGuestDriver {}
>  // SAFETY: This struct only contains integer types for which all bit patterns
>  // are valid.
>  unsafe impl FromBytes for UnloadingGuestDriver {}
> +
> +/// The host CPU architecture.
> +#[derive(Clone, Copy)]
> +pub(crate) enum HostArch {
> +    None = 0,
> +    X86_64 = 1,
> +    Ppc64le = 2,
> +    Arm = 3,
> +    Aarch64 = 4,
> +    Riscv64 = 5,
> +}
> +
> +// TODO[FPRI]: This is a temporary solution to be replaced with the corresponding derive macros once
> +// they land.
> +impl TryFrom<u32> for HostArch {
> +    type Error = Error;
> +
> +    fn try_from(value: u32) -> Result<Self> {
> +        match value {
> +            0 => Ok(Self::None),
> +            1 => Ok(Self::X86_64),
> +            2 => Ok(Self::Ppc64le),
> +            3 => Ok(Self::Arm),
> +            4 => Ok(Self::Aarch64),
> +            5 => Ok(Self::Riscv64),
> +            _ => Err(EINVAL),
> +        }
> +    }
> +}
> +
> +impl From<HostArch> for u32 {
> +    fn from(value: HostArch) -> Self {
> +        value as u32
> +    }
> +}
> +
> +nvkv_encode! {
> +    /// A GSP registry entry.
> +    struct RegKey {
> +        key_name: Key<&'static [u8], { Self::REGKEY_NAME_KEY }>,
> +        key_value: Key<u32, { Self::REGKEY_VALUE_U32_KEY }>,
> +    }
> +}
> +
> +impl RegKey {
> +    // Define the Key IDs read/written by GSP.
> +    const REGKEY_NAME_KEY: KeyId = 0x3070;
> +    const REGKEY_VALUE_U32_KEY: KeyId = 0x3071;

I guess the `REGKEY` prefix is unneeded here since it's the name of the
wrapping type. If we want a common prefix, let's use `KEY`, e.g.
`KEY_NAME`? Although we might not even need these at all - please see my
last comment on this patch.

> +}
> +
> +impl Encodable for KVVec<RegKey> {
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        for regkey in self {
> +            regkey.encode(encoder)?;
> +        }
> +        Ok(())
> +    }
> +}
> +
> +nvkv_encode! {
> +    /// SR-IOV virtual function information.
> +    struct VfInfo {
> +        total_vfs: Key<u32, { Self::VF_TOTAL_VFS_KEY }>,
> +        first_vf_offset: Key<u32, { Self::VF_FIRST_VF_OFFSET_KEY }>,
> +        flags: Key<u64, { Self::VF_FLAGS_KEY }>,
> +        first_bar0_address: Key<u64, { Self::VF_FIRST_BAR0_ADDRESS_KEY }>,
> +        first_bar1_address: Key<u64, { Self::VF_FIRST_BAR1_ADDRESS_KEY }>,
> +        first_bar2_address: Key<u64, { Self::VF_FIRST_BAR2_ADDRESS_KEY }>,
> +    }
> +}
> +
> +impl VfInfo {
> +    // Define the Key IDs read/written by GSP.
> +    const VF_TOTAL_VFS_KEY: KeyId = 0x0080;
> +    const VF_FIRST_VF_OFFSET_KEY: KeyId = 0x0081;
> +    const VF_FLAGS_KEY: KeyId = 0x1003;
> +    const VF_FIRST_BAR0_ADDRESS_KEY: KeyId = 0x1050;
> +    const VF_FIRST_BAR1_ADDRESS_KEY: KeyId = 0x1051;
> +    const VF_FIRST_BAR2_ADDRESS_KEY: KeyId = 0x1052;
> +}

That's a bit of boilerplate. Ideally we would have this `#[nvkv(key =
value)]` notation (without the bidirectional feature) you mentioned in
[1] that takes the literal value and defines a constant to access it,
but I guess that's more rework than we want for now.

[1] https://lore.kernel.org/DLIHSKKUVP7V.153Z4TTEYFL3Y@nvidia.com

> +
> +nvkv_encode! {
> +    /// Payload of the `GSP_INIT` command.
> +    // TODO: expect() doesn't work here due to Self:: reference, fixed in 1.97.0
> +    // https://github.com/rust-lang/rust/pull/154377
> +    #[cfg_attr(not(CONFIG_KUNIT), allow(dead_code))]
> +    struct GspInitRequest {
> +        pci_device_id: Key<u32, { Self::PCI_DEVICE_ID_KEY }>,
> +        pci_sub_device_id: Key<u32, { Self::PCI_SUBDEVICE_ID_KEY }>,
> +        pci_revision_id: Key<u32, { Self::PCI_REVISION_ID_KEY }>,
> +        pci_config_mirror_base: Key<u32, { Self::PCI_CONFIG_MIRROR_BASE_KEY }>,
> +        pci_config_mirror_size: Key<u32, { Self::PCI_CONFIG_MIRROR_SIZE_KEY }>,
> +        host_arch: Key<HostArch, { Self::HOST_ARCH_KEY }, u32>,
> +        bus_device_func: Key<u64, { Self::NV_DOMAIN_BUS_DEVICE_FUNC_KEY }>,
> +        regkeys: KVVec<RegKey>,
> +        vf_info: Option<VfInfo>,
> +    }
> +}
> +
> +impl GspInitRequest {
> +    // Define the Key IDs read/written by GSP.
> +    const PCI_DEVICE_ID_KEY: KeyId = 0x0001;
> +    const PCI_SUBDEVICE_ID_KEY: KeyId = 0x0002;
> +    const PCI_REVISION_ID_KEY: KeyId = 0x0003;
> +    const PCI_CONFIG_MIRROR_BASE_KEY: KeyId = 0x0010;
> +    const PCI_CONFIG_MIRROR_SIZE_KEY: KeyId = 0x0011;
> +    const HOST_ARCH_KEY: KeyId = 0x0070;
> +    const NV_DOMAIN_BUS_DEVICE_FUNC_KEY: KeyId = 0x1020;
> +}
> +
> +// Decode:
> +
> +// Should decode with UnknownKeyPolicy::Ignore.
> +nvkv_decode! {
> +    /// Schema for the `GSP_INIT` response.
> +    // TODO: expect() doesn't work here due to Self:: reference, fixed in 1.97.0
> +    // https://github.com/rust-lang/rust/pull/154377
> +    #[cfg_attr(not(CONFIG_KUNIT), allow(dead_code))]
> +    struct GspInitResponseSchema => GspInitResponse {
> +        gpu_name:
> +            Array<u8, { GspInitResponse::MAX_GPU_NAME_LEN }, { Self::GPU_NAME_STRING_KEY }>,
> +        fb_regions: Accumulated<FbRegionSchema>,
> +        bar1_pde_base: Required<u64, { Self::BAR1_PDE_BASE_KEY }>,
> +        vmmu_segment_size: Key<u64, { Self::VMMU_SEGMENT_SIZE_KEY }>,

Is this ok to have `vmmu_segment_size` not `Required`?

> +    }
> +}
> +
> +impl GspInitResponseSchema {
> +    // Define the Key IDs read/written by GSP.
> +    const GPU_NAME_STRING_KEY: KeyId = 0x2000;
> +    const BAR1_PDE_BASE_KEY: KeyId = 0x1020;
> +    const VMMU_SEGMENT_SIZE_KEY: KeyId = 0x1050;
> +}
> +
> +/// Payload of the `GSP_INIT` response.
> +struct GspInitResponse {
> +    gpu_name: ArrayVec<u8, { Self::MAX_GPU_NAME_LEN }>,
> +    fb_regions: KVVec<FbRegion>,
> +    bar1_pde_base: u64,
> +    vmmu_segment_size: u64,
> +}
> +
> +impl GspInitResponse {
> +    const MAX_GPU_NAME_LEN: usize = 64;
> +}
> +
> +nvkv_decode! {
> +    /// Schema for one FB region of the `GSP_INIT` response.
> +    struct FbRegionSchema => FbRegion {
> +        base: Required<u64, { Self::BASE_KEY }>,
> +        limit: Required<u64, { Self::LIMIT_KEY }>,
> +        flags: Required<FbRegionFlags, { Self::FLAGS_KEY }>,
> +        tag: Required<u32, { Self::TAG_KEY }>,
> +    }
> +}
> +
> +impl FbRegionSchema {
> +    // Define the Key IDs read/written by GSP.
> +    const BASE_KEY: KeyId = 0x1011;
> +    const LIMIT_KEY: KeyId = 0x1012;
> +    const FLAGS_KEY: KeyId = 0x0012;
> +    const TAG_KEY: KeyId = 0x0013;
> +}
> +
> +bitfield! {
> +    /// FB region attribute flags.
> +    struct FbRegionFlags(u32) {
> +        0:0 support_compressed => bool;
> +        1:1 support_iso => bool;
> +        2:2 protected => bool;
> +    }
> +}
> +
> +impl TryFrom<DecoderValue<'_>> for FbRegionFlags {
> +    type Error = Error;
> +
> +    fn try_from(value: DecoderValue<'_>) -> Result<Self> {
> +        if let DecoderValue::Scalar32(v) = value {
> +            Ok(v.into())
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +}
> +
> +/// One FB memory region.
> +struct FbRegion {
> +    base: u64,
> +    limit: u64,
> +    flags: FbRegionFlags,
> +    tag: u32,
> +}
> +
> +#[kunit_tests(nova_core_fw_commands)]
> +mod tests {

These tests below mostly re-check the same things as the previous
patches, only with different data. I am not sure they bring much new
coverage, except maybe nested structs (which we could/should also cover
in the previous patches anyway). I think we would have more value (and
less code) if we covered the things newly tested to the tests of patches
4-7, and dropped the test module here altogether. Because if we follow
the pattern, then we will repeat these tests again and again for every
command we support, which is going to result in tons of redundant tests
at the end of the day.

Another incentive for not having more tests here: they are the only
other user of the key constants, outside of the `nvkv_decode` and
`nvkv_encode` macros themselves. If we can get rid of them then the
macros become the only users of these consts, and in that case why not
replace

    pci_device_id: Key<u32, { Self::PCI_DEVICE_ID_KEY }>,
    ...
    const PCI_DEVICE_ID_KEY: KeyId = 0x0001;

with just

    pci_device_id: Key<u32, 0x0001>,

as it is clear from that line alone that `0x0001` is the key for
`pci_device_id`. Bonus, the unsightly brackets required because we
reference `Self` can also go away.