Re: [PATCH v2 5/8] gpu: nova-core: add NVKV decoder

On Wed Sep 9, 2026 at 10:13 AM JST, Eliot Courtney wrote:

On Wed Sep 9, 2026 at 9:51 AM JST, Alexandre Courbot wrote:

quoted

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

quoted

Add a decoder for NVKV. This is for receiving messages from GSP for
GMCAPI calls. The NVKV format essentially encodes a sequence of function
calls f(key, index, value). This decoder reads an encoded stream and
invokes a type implementing the new `Schema` visitor trait. The
`Schema` trait can either consume the value or not, which is useful for
composing Schemas. If a (key, index, value) is not consumed, error out
depending on `UnknownKeyPolicy`. Whether ignoring unknown keys is ok or
not is per each GMCAPI call.

Add kunit tests for the decoder.

Signed-off-by: Eliot Courtney <redacted>
---
 drivers/gpu/nova-core/gsp/nvkv.rs        |   3 +
 drivers/gpu/nova-core/gsp/nvkv/decode.rs | 265 +++++++++++++++++++++++++++++++
 2 files changed, 268 insertions(+)

diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
index a8e16687a134..cbeee7f376b6 100644
--- a/drivers/gpu/nova-core/gsp/nvkv.rs
+++ b/drivers/gpu/nova-core/gsp/nvkv.rs

@@ -27,6 +27,9 @@
 mod encode;
 pub(crate) use encode::*;
 
+mod decode;
+pub(crate) use decode::*;
+
 /// The allocator backing [`EncodedStream`].
 type StreamAllocator = KVmalloc;
 

diff --git a/drivers/gpu/nova-core/gsp/nvkv/decode.rs b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
new file mode 100644
index 000000000000..ceb97e73e100
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs

@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+
+#![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
+
+use kernel::prelude::*;
+
+use crate::gsp::nvkv::{
+    Index,
+    KeyId,
+    Op,
+    Opcode, //
+};
+use crate::num;
+
+/// A decoded NVKV value.
+#[derive(Copy, Clone)]
+pub(crate) enum DecoderValue<'a> {
+    Scalar32(u32),
+    Scalar64(u64),
+    Array8(&'a [u8]),
+    Array32(&'a [u32]),
+    Array64(&'a [u64]),
+}
+
+/// Implements `TryFrom` from the given `DecoderValue` variant to the given type.
+///
+/// `TryFrom` is used by the `Schema` implementations in this file to convert from the
+/// `DecoderValue`s into the types to store. Provide the implementations for basic types here.
+macro_rules! impl_try_from_decoder_value {
+    ($ty:ty, $variant:ident) => {
+        impl<'a> TryFrom<DecoderValue<'a>> for $ty {
+            type Error = Error;
+
+            fn try_from(value: DecoderValue<'a>) -> Result<Self> {
+                if let DecoderValue::$variant(v) = value {
+                    Ok(v)
+                } else {
+                    Err(EINVAL)
+                }
+            }
+        }
+    };
+}
+
+impl_try_from_decoder_value!(u32, Scalar32);
+impl_try_from_decoder_value!(u64, Scalar64);
+impl_try_from_decoder_value!(&'a [u8], Array8);
+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`.
+pub(crate) trait Schema {
+    type Target;
+
+    /// Visits one decoded pair. Returns `Ok(true)` if the schema consumed it.
+    fn visit<'a>(&mut self, key: KeyId, index: Index, value: DecoderValue<'a>) -> Result<bool>;
+
+    /// Returns an initializer that makes the decoded `Target`.
+    ///
+    /// After the returned initializer runs, the schema should be empty again.
+    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_;

Would it make sense to make `finish` consume `self`? Because "the schema
should be empty again" sounds like an implicit contract not everybody
will think about enforcing, which could be a source of subtle bugs.

Yeah, it would make sense, and that's what v1 of this series did. But, I
noticed that it forced materialisation of the Schema on the stack (and
the Schema can be large), even if you allocate the Schema using a Box.

There's two places where it materialises - in Decoder::decode and also
in Accumulated.

anyway, that's why I changed it to the valid-but-empty like convention.
Please LMK if you think there's a better trade off solution to avoiding
materialising this on the stack.

Indeed, I don't see a way around it. In this case can we make the
requirement stronger than "should be empty" in the doc? Because AFAIU
caller code depends on this behavior, although most of this is handled
by `nvkv_decode`, so we are working in a controlled environment here.