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

On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:

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

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

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.