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

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

On Thu Aug 27, 2026 at 11:12 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        |  12 +-
>  drivers/gpu/nova-core/gsp/nvkv/decode.rs | 480 ++++++++++++++++++++++++++++++-
>  2 files changed, 488 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
> index 10dcbb9e602c..7d58ca91cbc3 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))]

I am getting a build error on this patch:

error: unused import: `nvkv_encode`
  --> ../drivers/gpu/nova-core/gsp/nvkv/encode.rs:65:16
   |
65 | pub(crate) use nvkv_encode;
   |                ^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: unused import: `nvkv_decode`
   --> ../drivers/gpu/nova-core/gsp/nvkv/decode.rs:104:16
    |
104 | pub(crate) use nvkv_decode;
    |                ^^^^^^^^^^^

error: aborting due to 2 previous errors

>  #![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>  
>  use core::marker::PhantomData;
> @@ -21,7 +21,8 @@
>  use kernel::{
>      alloc::{
>          allocator::KVmalloc,
> -        Allocator, //
> +        Allocator,
> +        ArrayVec, //
>      },
>      bitfield,
>      num::Bounded,
> @@ -139,6 +140,13 @@ fn default() -> Self {
>      }
>  }
>  
> +/// A schema field for an array value under the NVKV key `KEY_ID`.
> +#[derive(Default)]
> +#[repr(transparent)]
> +pub(crate) struct Array<T: Default + Copy, const N: usize, const KEY_ID: KeyId> {
> +    vec: ArrayVec<T, N>,
> +}

Why is this not defined under `decoder` if it is only used there?

> +
>  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 ceb97e73e100..7f5310857764 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> @@ -3,16 +3,356 @@
>  
>  #![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>  
> -use kernel::prelude::*;
> +use core::convert::Infallible;
> +use core::marker::PhantomData;
> +
> +use kernel::{
> +    alloc::ArrayVec,
> +    prelude::*, //
> +};
> +use pin_init::init_array_from_fn;
>  
>  use crate::gsp::nvkv::{
> +    Array,
>      Index,
> +    Key,
>      KeyId,
>      Op,
>      Opcode, //
>  };
>  use crate::num;
>  
> +/// Defines a schema struct together with its [`Schema`] implementation that decodes into `$target`.
> +///
> +/// Each member of the struct should implement `Schema`. For every (key, index, value) triple
> +/// decoded from the NVKV stream, the generated parent `Schema` implementation will call each member
> +/// in declaration order with that triple. If a member consumes that triple, it will stop there.
> +/// Otherwise it will keep going until all members are tried.
> +///
> +/// The schema struct holds the state required by the schema implementation to do the decode. It's
> +/// recommended to use one of the existing Schema kinds (`Required`, `Accumulated`, `Key`, `Array`,
> +/// `Indexed`) for each member.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// nvkv_decode! {
> +///     struct RequestSchema => Request {
> +///         id: Required<u32, 0x0001>,
> +///         name: Array<u8, 64, 0x0002>,
> +///     }
> +/// }
> +/// ```
> +macro_rules! nvkv_decode {
> +    (
> +        $(#[$attr:meta])*
> +        $vis:vis struct $name:ident => $target:ident {
> +            $(
> +                $(#[$field_attr:meta])*
> +                $field_vis:vis $field:ident : $ty:ty
> +            ),* $(,)?
> +        }
> +    ) => {
> +        $(#[$attr])*
> +        $vis struct $name {
> +            $(
> +                $(#[$field_attr])*
> +                $field_vis $field: $ty,
> +            )*
> +        }
> +
> +        impl $crate::gsp::nvkv::Schema for $name {
> +            type Target = $target;
> +
> +            fn init() -> impl ::kernel::prelude::Init<Self> {
> +                ::pin_init::init!(Self {
> +                    $( $field <- <$ty as $crate::gsp::nvkv::Schema>::init(), )*
> +                })
> +            }
> +
> +            fn visit(
> +                &mut self,
> +                key: $crate::gsp::nvkv::KeyId,
> +                index: $crate::gsp::nvkv::Index,
> +                value: $crate::gsp::nvkv::DecoderValue<'_>,
> +            ) -> ::kernel::error::Result<bool> {
> +                Ok(false
> +                    $( || $crate::gsp::nvkv::Schema::visit(&mut self.$field, key, index, value)? )*)

Mmm looks like this is going to be `O(n)` with `n` being the number of
fields?

This is ok for a first implementation but eventually I hope we can
switch to a more efficient dispatch.

> +            }
> +
> +            #[inline(always)]

In this patch as well these should probably be just `#[inline]`.

> +            fn finish(
> +                &mut self,
> +            ) -> impl ::kernel::prelude::Init<Self::Target, ::kernel::error::Error> + '_ {
> +                let Self { $($field,)* } = self;
> +                ::kernel::try_init!(Self::Target {
> +                    $( $field <- $crate::gsp::nvkv::Schema::finish($field), )*
> +                }? ::kernel::error::Error)
> +            }
> +        }
> +
> +        impl ::core::default::Default for $name {
> +            fn default() -> Self {
> +                $crate::gsp::nvkv::assert_schema_size_reasonable::<Self>();
> +                Self {
> +                    $( $field: ::core::default::Default::default(), )*
> +                }
> +            }
> +        }
> +    };
> +}
> +pub(crate) use nvkv_decode;
> +
> +/// Asserts that a schema built by value is small enough.
> +pub(crate) fn assert_schema_size_reasonable<S>() {
> +    // Clippy triggers this even if the enclosing function is never called, so skip if clippy is on.
> +    const_assert!(
> +        cfg!(clippy) || size_of::<S>() <= 1024,
> +        "construct large schemas in place with `Schema::init` instead of `Default`"
> +    );
> +}
> +
> +impl<T: for<'a> TryFrom<DecoderValue<'a>, Error = Error> + Default, const KEY_ID: KeyId> Schema
> +    for Key<T, KEY_ID>
> +{
> +    type Target = T;
> +
> +    #[inline(always)]
> +    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool> {
> +        if key != KEY_ID {
> +            Ok(false)
> +        } else if index != Index::new::<0>() {
> +            // Single values being set must be at index 0.
> +            Err(EINVAL)
> +        } else {
> +            // Overwrite and take the latest value here.
> +            self.0 = value.try_into()?;
> +            Ok(true)
> +        }
> +    }
> +
> +    #[inline(always)]
> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> +        Ok(core::mem::take(&mut self.0))
> +    }
> +}
> +
> +impl<T: for<'a> TryFrom<DecoderValue<'a>, Error = Error>, const KEY_ID: KeyId> Schema
> +    for Key<Option<T>, KEY_ID>
> +{
> +    type Target = Option<T>;
> +
> +    #[inline(always)]
> +    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool> {
> +        if key != KEY_ID {
> +            Ok(false)
> +        } else if index != Index::new::<0>() {
> +            // Single values being set must be at index 0.
> +            Err(EINVAL)
> +        } else {
> +            // Overwrite and take the latest value here.
> +            self.0 = Some(value.try_into()?);
> +            Ok(true)
> +        }
> +    }
> +
> +    #[inline(always)]
> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> +        Ok(self.0.take())
> +    }
> +}
> +
> +impl<T: Default + Copy, const N: usize, const KEY_ID: KeyId> Schema for Array<T, N, KEY_ID>
> +where
> +    for<'a> &'a [T]: TryFrom<DecoderValue<'a>, Error = Error>,
> +{
> +    type Target = ArrayVec<T, N>;
> +
> +    fn init() -> impl Init<Self> {
> +        init!(Self {
> +            vec <- ArrayVec::init_with::<Infallible>(|_| Ok(())),
> +        })
> +    }
> +
> +    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool> {
> +        if key != KEY_ID {
> +            return Ok(false);
> +        }
> +        // Require to be at index 0
> +        if index != Index::new::<0>() {
> +            return Err(EINVAL);
> +        }
> +        // Reject oversized and take the latest value.
> +        self.vec.clear();
> +        self.vec.extend_from_slice(value.try_into()?)?;
> +        Ok(true)
> +    }
> +
> +    #[inline(always)]
> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> +        ArrayVec::init_with(move |dst| {
> +            dst.extend_from_slice(&self.vec)?;
> +            self.vec.clear();
> +            Ok(())
> +        })
> +    }
> +}
> +
> +/// 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>);
> +
> +impl<T: for<'a> TryFrom<DecoderValue<'a>, Error = Error>, const KEY_ID: KeyId> Schema
> +    for Required<T, KEY_ID>
> +{
> +    type Target = T;
> +
> +    #[inline(always)]
> +    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool> {
> +        self.0.visit(key, index, value)
> +    }
> +
> +    #[inline(always)]
> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> +        (self.0).0.take().ok_or(EINVAL)
> +    }
> +}
> +
> +impl<T, const KEY_ID: KeyId> Default for Required<T, KEY_ID> {
> +    fn default() -> Self {
> +        Self(None.into())
> +    }
> +}
> +
> +/// Expects objects specified sequentially with index starting from zero.
> +pub(crate) struct Accumulated<S: Schema> {
> +    current_index: Index,
> +    current: S,
> +    current_started: bool,
> +    next: S,
> +    accumulated: KVVec<S::Target>,
> +}
> +
> +impl<S: Schema + Default> Accumulated<S> {
> +    /// Creates an empty accumulator.
> +    pub(crate) fn new() -> Self {
> +        Self {
> +            current_index: Index::new::<0>(),
> +            current: S::default(),
> +            current_started: false,
> +            next: S::default(),
> +            accumulated: KVVec::new(),

Do we want to call `assert_schema_size_reasonable` somewhere here as
well? Also, should this be a `Default` implementation?

> +        }
> +    }
> +
> +    fn take_vec(&mut self) -> Result<KVVec<S::Target>> {
> +        if self.current_started {
> +            self.accumulated
> +                .try_push_init(self.current.finish(), GFP_KERNEL)?;
> +            self.current_started = false;
> +        }
> +        self.current_index = Index::new::<0>();
> +        Ok(core::mem::take(&mut self.accumulated))
> +    }

This seems to be only called by `finish`, let's inline it there?

> +}
> +
> +impl<S: Schema + Default> Schema for Accumulated<S> {

If this ok that this doesn't provide an `init` implementation? Because
the default one returns a value on the stack, which IIUC can grow rather
consequently for an `Accumulated`?

> +    type Target = KVVec<S::Target>;
> +
> +    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool> {
> +        if index != self.current_index {
> +            if !self.next.visit(key, Index::new::<0>(), value)? {
> +                // Unrelated key to us.
> +                return Ok(false);
> +            }
> +
> +            // Require that objects at index k have all their keys sent before the k + 1 th object
> +            // can be completed. Require that objects are sent contiguously in order from index 0.
> +            if !self.current_started || index != self.current_index + 1 {
> +                return Err(EINVAL);
> +            }
> +
> +            // The current value must be finished. Push it and swap in `next`.
> +            self.accumulated
> +                .try_push_init(self.current.finish(), GFP_KERNEL)?;
> +            core::mem::swap(&mut self.current, &mut self.next);
> +            self.current_started = true;
> +            self.current_index = index;
> +            Ok(true)

I don't quite understand how this method works, notably how
`current_index` evolves. This might require more documentation on
`Accumulated` itself.

> +        } else {
> +            let consumed = self.current.visit(key, Index::new::<0>(), value)?;
> +            self.current_started |= consumed;
> +            Ok(consumed)
> +        }
> +    }
> +
> +    #[inline(always)]
> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> +        self.take_vec()
> +    }
> +}
> +
> +impl<S: Schema + Default> Default for Accumulated<S> {
> +    fn default() -> Self {
> +        Self::new()
> +    }
> +}
> +
> +/// A schema field that scatters indexed values into an array of `N` slots.
> +#[repr(transparent)]
> +pub(crate) struct Indexed<T, const N: usize, const KEY_ID: KeyId, As = T>([T; N], PhantomData<As>);

Can we elaborate a bit on what `As` is supposed to be? Not only on this
site, but generally speaking. I have a hard time coming with a
consistent definition, so a comment would help the reader forge their
understanding.

> +
> +/// Copies `elems`, converted to `T`, into `slots` at `start`.
> +///
> +/// Fails with `EINVAL` if the window does not fit in `slots`.
> +fn scatter_window<T: From<As>, As: Copy>(slots: &mut [T], start: usize, elems: &[As]) -> Result {
> +    let end = start.checked_add(elems.len()).ok_or(EINVAL)?;
> +    // Reject indices outside of the declared array size.
> +    let dst = slots.get_mut(start..end).ok_or(EINVAL)?;
> +    for (d, &e) in dst.iter_mut().zip(elems) {
> +        *d = T::from(e);
> +    }
> +    Ok(())
> +}
> +
> +impl<T, const N: usize, const KEY_ID: KeyId, As> Schema for Indexed<T, N, KEY_ID, As>

Same question as `Accumulated` about the lack of an `init` method -
maybe we can use `init_array_from_fn` to avoid a stack copy.

Actually that makes me think that maybe the default `Schema::init`
implementation is not such good an idea, because it makes us overlook
types where we should override it.

> +where
> +    T: From<As> + Default,
> +    As: Copy + for<'a> TryFrom<DecoderValue<'a>, Error = Error>,
> +    for<'a> &'a [As]: TryFrom<DecoderValue<'a>, Error = Error>,
> +{
> +    type Target = [T; N];
> +
> +    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool> {
> +        if key != KEY_ID {
> +            return Ok(false);
> +        }
> +        let start = index.cast::<usize>().get();
> +        // Accept both scalar vs scattered array setting for flexibility.
> +        match <&[As]>::try_from(value) {
> +            Ok(elems) => scatter_window(&mut self.0, start, elems)?,
> +            Err(_) => scatter_window(&mut self.0, start, &[As::try_from(value)?])?,
> +        }
> +        Ok(true)
> +    }
> +
> +    #[inline(always)]
> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> +        init_array_from_fn(|i| Ok::<_, Error>(core::mem::take(&mut self.0[i])))
> +    }
> +}
> +
> +impl<T: Default + Copy, const N: usize, const KEY_ID: KeyId, As> Default
> +    for Indexed<T, N, KEY_ID, As>
> +{
> +    fn default() -> Self {
> +        assert_schema_size_reasonable::<Self>();
> +        Self([T::default(); N], PhantomData)
> +    }

Mmm that could be a pretty large object. Where are these `default`
methods called? Do we want to leverage `init` instead?

<...>
> +    // Tests that a schema too large for the stack decodes on the heap.
> +    #[test]
> +    fn decode_large_schema_on_heap() -> Result {
> +        const BLOB_KEY: KeyId = 0x1400;
> +        const BLOB_VALUE: &[u8] = &[0xab; 100];
> +
> +        nvkv_decode! {
> +            struct BigSchema => BigDecodeable {
> +                blob: Array<u8, 2048, { BLOB_KEY }>,
> +            }
> +        }
> +
> +        struct BigDecodeable {
> +            blob: ArrayVec<u8, 2048>,
> +        }
> +
> +        let mut encoder = Encoder::new();
> +        encoder.encode_array8(BLOB_KEY, Index::new::<0>(), BLOB_VALUE)?;
> +        let serialized = encoder.finish();
> +
> +        let mut schema = KBox::init(BigSchema::init(), GFP_KERNEL)?;
> +        let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
> +        let decoded = KBox::try_init(decoder.decode(&mut *schema)?, GFP_KERNEL)?;
> +
> +        assert_eq!(*decoded.blob, *BLOB_VALUE);
> +        Ok(())
> +    }

Same as the encoder, it would be nice to exercise the error paths a bit
more in the tests.