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

[Eliot Courtney]

On Mon Sep 14, 2026 at 12:46 PM JST, Alexandre Courbot wrote:
> 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@xxxxxxxxxx>
>> ---
>>  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

Thanks for catching this. This builds on 1.85.0 without issue, but I
checked on 1.98.1 and it fails to build. I'll add building with stable
to my checklist.

>
>>  #![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?

This will be used soon. For example [1] uses it.

[1]: https://lore.kernel.org/all/20260905081116.106613-8-zhiw@xxxxxxxxxx/

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

I thought quite a bit about this while writing this code, since we need
the escape hatch to imperative decode (custom Schema impl basically). To
be able to get it down to a match on the key, we need to know ahead of
time which keys a Schema will consume. That duplicates the info from the
visit() implementation.

I thought up a few methods but it's unclear to me which one is best, so
I just left it for now. Please LMK if you think this is urgent, I can
try in a follow up to improve this. Here are my ideas (when I say O(1)
lookup I mean modulo how the compiler decides to do it with the set of
key IDs it gets):

1. current code - just visit()
pros: key source of truth not duplicates
cons: O(field) visit as you say

2. Associated const KEY_ID: Option<KeyId> - None if a Schema accepts multiple keys.
You can match on each associated const in the macro.
pros: O(1) if the current key goes to a field with KEY_ID = Some(...)
cons: O(#fields accepting multiple keys) if current key is one of them

3. fn accepts() -> bool
You can match on `if F::accepts(key)` for each field. We could potentially make
this const with Gary's const traits polyfill.
pros: O(1) if you write an inline-able+optimizable implementation.

4. Associated const KEYS table; use tricks to concat tables
pros: O(1) lookup 
cons: actually MSRV can't get this to optimize down to O(1) 
  if you use slice::contains(), but stable can.

I don't like #2. With the current #1 we can decide later how to optimize.
#3 and #4 feel mostly equal to me, maybe #3 is slightly better.

>
>> +            }
>> +
>> +            #[inline(always)]
>
> In this patch as well these should probably be just `#[inline]`.

Done.

[...]
>> +/// 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?

Think we can just remove the ability to construct this without using
init(). Then callers can use stack_pin_init! if they really want it on
the stack.

>
>> +        }
>> +    }
>> +
>> +    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?

Done.

>
>> +}
>> +
>> +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`?

Yeah. So it happens that the stack copies are elided in this particular
case. But I think it's better anyway to do it as you suggest.

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

I added some documentation about how it works. But this is just an
implementation of a finite state machine which tracks the completion of
each child Schema based on the index advancing. It needs some
bookkeeping to handle edge cases like you didn't receive anything / you
got to the end (`current_started`).

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

It's documented on `Key` but not here, let me add a link to it.

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

Yeah agreed on all points.

>
>> +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?

Yerp

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

Will do.