Re: [PATCH v3 7/8] gpu: nova-core: add NVKV typed decoding

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

On Mon Sep 28, 2026 at 5:42 PM JST, Eliot Courtney wrote:
> Similar to the typed encoding layer, add some decoding type machinery.
> Add a simple macro `nvkv_decode!` which implements `Schema` for a struct
> by composing visit calls to each member. Add some common `Schema` kinds,
> such as `Array` which collects an array value into a fixed maximum size
> array, and `Required` which fails a decode if the value is not sent.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/nvkv.rs        |  11 +-
>  drivers/gpu/nova-core/gsp/nvkv/decode.rs | 622 ++++++++++++++++++++++++++++++-
>  2 files changed, 628 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
> index 7ac3a459a98b..5791df07a7fa 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
> @@ -9,7 +9,7 @@
>  //! function calls will map to some struct - for example, f(GPU_NAME_STRING_KEY, 0, b"some gpu")
>  //! naturally maps to storing a &str with the GPU name.
>  
> -#![expect(unused_imports)]
> +#![cfg_attr(not(CONFIG_KUNIT), expect(unused_imports))]
>  #![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>  
>  use core::{
> @@ -23,7 +23,8 @@
>  use kernel::{
>      alloc::{
>          allocator::KVmalloc,
> -        Allocator, //
> +        Allocator,
> +        ArrayVec, //
>      },
>      bitfield,
>      num::Bounded,
> @@ -148,6 +149,12 @@ fn default() -> Self {
>      }
>  }
>  
> +/// A schema field for an array value under the NVKV key `KEY_ID`.
> +#[repr(transparent)]
> +pub(crate) struct Array<T: Default + Copy, const N: usize, const KEY_ID: KeyId> {
> +    vec: ArrayVec<T, N>,
> +}

I still don't see why this needs to be declared in this module when no
encoder element ever uses it, and the module reexports everything from
`decode` anyway. Can this be moved next to the other schema kinds?

> +
>  bitfield! {
>      /// The op word that starts each NVKV operation.
>      struct Op(u64) {
> diff --git a/drivers/gpu/nova-core/gsp/nvkv/decode.rs b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> index c4c24fe1108e..24dad31296cb 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> @@ -3,11 +3,22 @@
>  
>  #![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>  
> -use kernel::prelude::*;
> +use core::{
> +    convert::Infallible,
> +    marker::PhantomData, //
> +};
> +
> +use kernel::{
> +    alloc::ArrayVec,
> +    prelude::*, //
> +};
> +use pin_init::init_array_from_fn;
>  
>  use crate::{
>      gsp::nvkv::{
> +        Array,
>          Index,
> +        Key,
>          KeyId,
>          Op,
>          Opcode, //
> @@ -15,6 +26,353 @@
>      num, //
>  };
>  
> +/// Defines a schema struct together with its [`Schema`] and [`Visit`] implementations that decode
> +/// into `$target`.
> +///
> +/// Each member of the struct should implement [`Schema`] and [`Visit`]. For every (key, index,
> +/// value) triple decoded from the NVKV stream, the generated parent `Visit` implementation will

nit: let's put backticks around (key, index, value) and link to `Visit`
(and other types mentioned in this doccomment, and possibly elsewhere in
this patch).

<...>
> +/// A schema field for a key that must be present.
> +///
> +/// `finish` fails with `EINVAL` if no value arrived for the key.
> +#[repr(transparent)]
> +pub(crate) struct Required<T, const KEY_ID: KeyId>(Key<Option<T>, KEY_ID>);

I just noticed that the schema field types are conflating several
concepts together. On the one hand, you have types associating a key to
some kind of storage (Key, Array, Indexed - let's call these leaf
types), and on the other what looks like modifiers on said leaf types
(Accumulated).

Which leaves `Required` somewhere in between, as it takes a key
parameter and can only be applied to value types, i.e. you currently
cannot have a `Required<Array<...>>`. But `Accumulated<Array<...>>` does
work IIUC.

So I think `Required` should work the same way as `Accumulated`, i.e.
just wrap it around the type you want to make required, instead of
switching the leaf type.

And for symmetry, we should also have an `Optional` wrapper type, so we
can also make fields optional even if they are not `Key`s instead of
having specific behavior for `Key<Option>` (which again is not
composable).

Which would give us 3 leaf types (Key, Array, Indexed) and 3 modifiers
(Required, Optional, Accumulated), with a non-wrapped leaf meaning
"default if unspecified" - which is easier to understand than the
current types where `Required` is a synonym for "a `Key` except it's
required".

This means `Required` would then be used like `Required<Key<...>>`,
which is a bit more verbose, but again nicer to read.

<...>
>  /// A decoded NVKV value.
>  #[derive(Copy, Clone, Debug, PartialEq, Eq)]
>  pub(crate) enum DecoderValue<'a> {
> @@ -51,7 +409,16 @@ fn try_from(value: DecoderValue<'a>) -> Result<Self> {
>  impl_try_from_decoder_value!(&'a [u32], Array32);
>  impl_try_from_decoder_value!(&'a [u64], Array64);
>  
> -/// A visitor that consumes decoded NVKV and produces a `Target`.

Why change this doccomment? Let's introduce its final form in patch 5.

> +/// Lets `Key<Option<T>, KEY_ID>` accept whatever `Key<T, KEY_ID>` accepts.
> +impl<'a, T: TryFrom<DecoderValue<'a>, Error = Error>> TryFrom<DecoderValue<'a>> for Option<T> {
> +    type Error = Error;
> +
> +    fn try_from(value: DecoderValue<'a>) -> Result<Self> {
> +        T::try_from(value).map(Some)
> +    }
> +}

IIUC this block could go away if we add an `Optional` wrapper suggested
above.

> +
> +/// The state of one NVKV decode operation which produces a target value `Target`.
>  pub(crate) trait Schema {
>      type Target;
>  
> @@ -65,7 +432,12 @@ fn init() -> impl Init<Self>
>  
>      /// Returns an initializer that makes the decoded `Target`.
>      ///
> -    /// After the returned initializer runs, the schema should be empty again.
> +    /// After the returned initializer runs successfully, the schema must be empty again. For
> +    /// example, this is required by [`Accumulated`] which finishes one object and then decodes the
> +    /// next one with the same schema. Implementations generated by `nvkv_decode!` meet this
> +    /// requirement. If the initializer fails, the `Schema` can be in a valid but non-fresh state.
> +    /// Taking `self` instead of `&mut self` would avoid this contract, but it forces a copy of the
> +    /// schema onto the stack.

Let's not cite specific implementations in the doccomment of a trait. We
want to say that the schema *must* be empty (as opposed to "should")
from patch 5, and that should be clear enough and sufficient. I'd also
move the last sentence to patch 5, and drop the rest.

>      fn finish(&mut self) -> impl Init<Self::Target, Error> + '_;
>  }
>  
> @@ -295,6 +667,133 @@ fn visit(&mut self, key: KeyId, index: Index, value: DecoderValue<'d>) -> Result
>          Ok(())
>      }
>  
> +    // Tests that decoding via the `nvkv_decode!` macro works correctly.
> +    #[test]
> +    fn decode_typed_struct() -> Result {
> +        const SCALAR32_KEY: KeyId = 0x1234;
> +        const SCALAR64_KEY: KeyId = 0x1235;
> +        const ARRAY8_KEY: KeyId = 0x1236;
> +        const ARRAY32_KEY: KeyId = 0x1237;
> +        const ARRAY64_KEY: KeyId = 0x1238;
> +        const OPT_PRESENT_KEY: KeyId = 0x1239;
> +        const OPT_ABSENT_KEY: KeyId = 0x123a;
> +        const X_KEY: KeyId = 0x0100;
> +        const Y_KEY: KeyId = 0x0101;
> +        const SLOT_KEY: KeyId = 0x0200;
> +
> +        const SCALAR32_VALUE: u32 = 0x89ab_cdef;
> +        const SCALAR64_VALUE: u64 = 0x0123_4567_89ab_cdef;
> +        const ARRAY8_VALUE: &[u8] = &[0x12, 0x34, 0x56];
> +        const ARRAY32_VALUE: &[u32] = &[0x0123_4567, 0x89ab_cdef];
> +        const ARRAY64_VALUE: &[u64] = &[0x0123_4567_89ab_cdef, 0xfedc_ba98_7654_3210];
> +        const OPT_PRESENT_VALUE: u32 = 0x55;
> +
> +        nvkv_decode! {
> +            struct PairSchema => Pair {
> +                x: Required<u32, X_KEY>,
> +                y: Required<u32, Y_KEY>,
> +            }
> +        }
> +
> +        struct Pair {
> +            x: u32,
> +            y: u32,
> +        }
> +
> +        nvkv_decode! {
> +            struct TestSchema => TestDecodeable {
> +                scalar32: Required<u32, SCALAR32_KEY>,
> +                scalar64: Required<u64, SCALAR64_KEY>,
> +                array8: Array<u8, 64, ARRAY8_KEY>,
> +                array32: Array<u32, 64, ARRAY32_KEY>,
> +                array64: Array<u64, 32, ARRAY64_KEY>,
> +                opt_present: Key<Option<u32>, OPT_PRESENT_KEY>,
> +                opt_absent: Key<Option<u32>, OPT_ABSENT_KEY>,
> +                pairs: Accumulated<PairSchema>,
> +                slots: Indexed<u32, 4, SLOT_KEY>,
> +            }
> +        }
> +
> +        struct TestDecodeable {
> +            scalar32: u32,
> +            scalar64: u64,
> +            array8: ArrayVec<u8, 64>,
> +            array32: ArrayVec<u32, 64>,
> +            array64: ArrayVec<u64, 32>,
> +            opt_present: Option<u32>,
> +            opt_absent: Option<u32>,
> +            pairs: KVVec<Pair>,
> +            slots: [u32; 4],
> +        }
> +
> +        let index0 = Index::new::<0>();
> +        let index1 = Index::new::<1>();
> +        let index2 = Index::new::<2>();
> +        let mut encoder = Encoder::new();
> +        encoder.encode_u32(SCALAR32_KEY, index0, SCALAR32_VALUE)?;
> +        encoder.encode_u64(SCALAR64_KEY, index0, SCALAR64_VALUE)?;
> +        encoder.encode_array8(ARRAY8_KEY, index0, ARRAY8_VALUE)?;
> +        encoder.encode_array32(ARRAY32_KEY, index0, ARRAY32_VALUE)?;
> +        encoder.encode_array64(ARRAY64_KEY, index0, ARRAY64_VALUE)?;
> +        encoder.encode_u32(OPT_PRESENT_KEY, index0, OPT_PRESENT_VALUE)?;
> +        encoder.encode_u32(X_KEY, index0, 1)?;
> +        encoder.encode_u32(Y_KEY, index0, 2)?;
> +        encoder.encode_u32(SLOT_KEY, index1, 20)?;
> +        encoder.encode_u32(X_KEY, index1, 3)?;
> +        encoder.encode_u32(Y_KEY, index1, 4)?;
> +        encoder.encode_u32(SLOT_KEY, index0, 10)?;
> +        encoder.encode_array32(SLOT_KEY, index2, &[30, 40])?;
> +        let serialized = encoder.finish();
> +
> +        let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
> +        let mut schema = KBox::init(TestSchema::init(), GFP_KERNEL)?;
> +        let decoded = KBox::try_init(decoder.decode(&mut *schema)?, GFP_KERNEL)?;
> +
> +        assert_eq!(decoded.scalar32, SCALAR32_VALUE);
> +        assert_eq!(decoded.scalar64, SCALAR64_VALUE);
> +        assert_eq!(*decoded.array8, *ARRAY8_VALUE);
> +        assert_eq!(*decoded.array32, *ARRAY32_VALUE);
> +        assert_eq!(*decoded.array64, *ARRAY64_VALUE);
> +        assert_eq!(decoded.opt_present, Some(OPT_PRESENT_VALUE));
> +        assert_eq!(decoded.opt_absent, None);
> +        assert_eq!(decoded.pairs.len(), 2);
> +        assert_eq!(decoded.pairs[0].x, 1);
> +        assert_eq!(decoded.pairs[0].y, 2);
> +        assert_eq!(decoded.pairs[1].x, 3);
> +        assert_eq!(decoded.pairs[1].y, 4);
> +        assert_eq!(decoded.slots, [10, 20, 30, 40]);
> +
> +        Ok(())
> +    }
> +
> +    // Tests that a schema too large for the stack decodes on the heap.

How are we testing this? Do we expect the stack to explode as a test
failure? That should be guaranteed by the use of `KBox` and `init`, so I
am not sure there is much to test here...

But if you decide to keep it, let's at least size the schema so the
stack *actually* explodes: 2KB is not enough, we want at least 16KB, and
for good measure let's go with at least 128KB to be sure. Otherwise I
think it's fine to drop this test.