On Mon Sep 28, 2026 at 5:42 PM JST, Eliot Courtney wrote:
> 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` and `Visit` trait. The
> `Visit` 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 <[email protected]>
> ---
> drivers/gpu/nova-core/gsp/nvkv.rs | 3 +
> drivers/gpu/nova-core/gsp/nvkv/decode.rs | 487
> +++++++++++++++++++++++++++++++
> 2 files changed, 490 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs
> b/drivers/gpu/nova-core/gsp/nvkv.rs
> index 0957dce92f96..10f7a16ffc23 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
> @@ -29,6 +29,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..c4c24fe1108e
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> @@ -0,0 +1,487 @@
> +// 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, //
> + },
> + num, //
> +};
> +
> +/// A decoded NVKV value.
> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
> +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.
nit: proper doclinks for referenced types (recurring problem throughout
the file).
> +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;
> +
> + /// Returns an initializer that creates an empty schema in place.
> + ///
> + /// Use [`KBox::init`] for the heap or `stack_pin_init!` for the stack
> (if sure that the value
> + /// is small enough to fit).
> + fn init() -> impl Init<Self>
> + where
> + Self: Sized;
> +
> + /// 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> + '_;
> +}
> +
> +/// A visitor that consumes decoded NVKV from a stream.
Note that `Schema` is also defined as a "visitor that consumes decoded
NVKV", which makes things a bit confusing. I guess that's because both
traits implement different aspects of what a typical visitor does, but
clarifying their relationship (and the reason for splitting) would be
helpful.
> +///
> +/// A schema that doesn't need to borrow data from the stream can implement
> this for all `'data`
> +/// lifetimes, avoiding having to carry the lifetime parameter. A schema
> that borrows from the
> +/// stream directly should implements it for its own lifetime only.
> +pub(crate) trait Visit<'data> {
Should this be named `Visitor` instead of `Visit`? Trait names should be
nouns rather than verbs, and this would make the `Decoder::visit`
method's prototype read more clearly imho. If you intended to use
`Visit` as a noun, I don't think that works either since the `visit`
method is potentially called many times (so it is not a single visit).
> + /// Visits one decoded pair. Returns `Ok(true)` if the schema consumed
> it.
> + fn visit(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'data>) -> Result<bool>;
> +}
> +
> +/// A read position in an NVKV stream.
> +struct Cursor<'a> {
> + data: &'a [u64],
> +}
> +
> +impl<'a> Cursor<'a> {
> + /// Creates a cursor at the start of `data`.
> + fn new(data: &'a [u64]) -> Self {
> + Self { data }
> + }
> +
> + /// Returns `true` if no `u64` values remain.
> + fn is_empty(&self) -> bool {
> + self.data.is_empty()
> + }
> +
> + /// Takes the next `u64`.
> + fn take_u64(&mut self) -> Result<u64> {
> + // PANIC: `take_u64s(1)` returns exactly one element on success.
> + Ok(self.take_u64s(1)?[0])
> + }
> +
> + /// Takes `count` bytes. If `count` is not a multiple of 8 (`u64` size),
> bytes are discarded up
> + /// to the next multiple.
> + fn take_u8s(&mut self, count: usize) -> Result<&'a [u8]> {
> + let values = self.take_u64s(count.div_ceil(8))?;
s/8/size_of::<u64>()
> + values.as_bytes().get(..count).ok_or(EINVAL)
> + }
> +
> + /// Takes `count` 32-bit values. If `count` is not a multiple of 2
> (`u64` size), bytes are
> + /// discarded up to the next multiple.
> + fn take_u32s(&mut self, count: usize) -> Result<&'a [u32]> {
> + let values = self.take_u64s(count.div_ceil(2))?;
... and maybe `size_of::<u64>() / size_of::<u32>()` here?
> + <[u32]>::ref_from_prefix_with_elems(values.as_bytes(), count)
> + .map(|(elems, _)| elems)
> + .map_err(|_| EINVAL)
> + }
> +
> + /// Takes `count` `u64` values, or fails with `EINVAL` if fewer remain.
> + fn take_u64s(&mut self, count: usize) -> Result<&'a [u64]> {
> + let (prefix, suffix) =
> self.data.split_at_checked(count).ok_or(EINVAL)?;
> + self.data = suffix;
> + Ok(prefix)
> + }
> +}
> +
> +/// A decoder for an NVKV stream.
> +pub(crate) struct Decoder<'a> {
> + data: &'a [u64],
> + policy: UnknownKeyPolicy,
> +}
> +
> +impl<'a> Decoder<'a> {
> + /// Creates a decoder for `data` that handles unknown keys per `policy`.
> + pub(crate) fn new(data: &'a [u64], policy: UnknownKeyPolicy) -> Self {
> + Self { data, policy }
> + }
> +
> + fn visit<S: Visit<'a>>(
Although private this method would benefit from having a short documentation.
> + &self,
> + schema: &mut S,
> + key: KeyId,
> + index: Index,
> + value: DecoderValue<'a>,
> + ) -> Result {
> + let consumed = schema.visit(key, index, value)?;
> + if !consumed && self.policy == UnknownKeyPolicy::Error {
> + Err(EINVAL)
> + } else {
> + Ok(())
> + }
> + }
> +
> + fn seq_key(base: KeyId, offset: usize) -> Result<KeyId> {
Same here.
> + base.checked_add(KeyId::try_from(offset)?).ok_or(EINVAL)
> + }
> +
> + /// Decodes every pair into `schema` and returns the result of
> [`Schema::finish`].
> + pub(crate) fn decode<'s, S: Schema + Visit<'a>>(
> + &self,
> + schema: &'s mut S,
> + ) -> Result<impl Init<S::Target, Error> + 's> {
> + let mut cursor = Cursor::new(self.data);
> + while !cursor.is_empty() {
> + let op: Op = cursor.take_u64()?.into();
> +
> + let key = op.key().into();
Tip: you should be able to declare the `key` field as follows
15:0 key => KeyId
... and obtain a `KeyId` directly, making the call to `into` unnecessary.
> + let index = op.index();
> + let op_value: u32 = op.value().into();
Same here with `u32`.
> + match op.opcode()? {
> + Opcode::Imm32 => {
> + self.visit(schema, key, index,
> DecoderValue::Scalar32(op_value))?;
> + }
> + Opcode::Seq32 => {
> + let values =
> cursor.take_u32s(num::u32_as_usize(op_value))?;
nit: let's use the `IntoSafeCast` trait instead since this code does not
run in const context.
> + for (i, &value) in values.iter().enumerate() {
> + let key = Self::seq_key(key, i)?;
> + self.visit(schema, key, index,
> DecoderValue::Scalar32(value))?;
> + }
> + }
> + Opcode::Seq64 => {
> + let values =
> cursor.take_u64s(num::u32_as_usize(op_value))?;
> + for (i, &value) in values.iter().enumerate() {
> + let key = Self::seq_key(key, i)?;
> + self.visit(schema, key, index,
> DecoderValue::Scalar64(value))?;
> + }
> + }
> + Opcode::Array8 => {
> + let value =
> cursor.take_u8s(num::u32_as_usize(op_value))?;
> + self.visit(schema, key, index,
> DecoderValue::Array8(value))?;
> + }
> + Opcode::Array32 => {
> + let value =
> cursor.take_u32s(num::u32_as_usize(op_value))?;
> + self.visit(schema, key, index,
> DecoderValue::Array32(value))?;
> + }
> + Opcode::Array64 => {
> + let value =
> cursor.take_u64s(num::u32_as_usize(op_value))?;
> + self.visit(schema, key, index,
> DecoderValue::Array64(value))?;
> + }
> + };
> + }
> + Ok(schema.finish())
> + }
> +}
> +
> +/// This is defined per call.
> +#[derive(Debug, Clone, Copy, PartialEq, Eq)]
> +pub(crate) enum UnknownKeyPolicy {
> + Ignore,
> + Error,
> +}
Documentation explaining the effect of each variant would be nice.
> +
> +#[kunit_tests(nova_core_nvkv_decode)]
> +mod tests {
> + use super::*;
> +
> + use crate::gsp::nvkv::Encoder;
> +
> + // Tests that basic decoding into a manually implemented `Schema` works
> correctly.
> + #[test]
> + fn decode_raw_schema() -> Result {
> + // Decodes an IMM32 pair and a SEQ64 pair (the encoder emits a u64
> as a single-element
> + // SEQ64) with a hand written `Schema`. Keys and value constants
> chosen to distinguish e.g.
> + // saving the wrong value to the wrong location.
> + const SCALAR32_KEY: KeyId = 0x1001;
> + const SCALAR64_KEY: KeyId = 0x1002;
> + const UNKNOWN_KEY: KeyId = 0x2001;
> +
> + const SCALAR32_VALUE: u32 = 0x1111_2222;
> + const SCALAR64_VALUE: u64 = 0x3333_4444_5555_6666;
> +
> + // The output type of the hand written `Schema`. In this case, we
> can have it also implement
> + // `Schema` on itself rather than having a separate carrier type,
> since the `Schema`
> + // implementation is completely stateless.
> + #[derive(Default)]
> + struct RawSchema {
> + scalar32: u32,
> + scalar64: u64,
> + }
> +
> + impl Schema for RawSchema {
> + type Target = Self;
> +
> + fn init() -> impl Init<Self> {
> + Self::default()
> + }
> +
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + Ok(core::mem::take(self))
> + }
> + }
> +
> + impl<'d> Visit<'d> for RawSchema {
> + fn visit(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'d>) -> Result<bool> {
> + if index != Index::new::<0>() {
> + return Err(EINVAL);
> + }
> + match key {
> + SCALAR32_KEY => self.scalar32 = value.try_into()?,
> + SCALAR64_KEY => self.scalar64 = value.try_into()?,
> + _ => return Ok(false),
> + }
> + Ok(true)
> + }
> + }
> +
> + let mut encoder = Encoder::new();
> + encoder.encode_u32(SCALAR32_KEY, Index::new::<0>(), SCALAR32_VALUE)?;
> + encoder.encode_u64(SCALAR64_KEY, Index::new::<0>(), SCALAR64_VALUE)?;
> + let serialized = encoder.finish();
> +
> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
> + let mut schema = KBox::init(RawSchema::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);
Nice test!
> +
> + // An unknown key should fail with under `UnknownKeyPolicy::Error`
> and be skipped under
> + // `UnknownKeyPolicy::Ignore`.
> + let mut encoder = Encoder::new();
> + encoder.encode_u32(UNKNOWN_KEY, Index::new::<0>(), 1)?;
> +
> + let serialized = encoder.finish();
> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
> + let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
> + assert!(decoder.decode(&mut *schema).is_err());
> +
> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Ignore);
> + let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
> + let decoded = KBox::try_init(decoder.decode(&mut *schema)?,
> GFP_KERNEL)?;
> + assert_eq!(decoded.scalar32, 0);
... this part looks like it should be its own test though. That's
trivial to achieve if you declare `RawSchema` at the module-level
instead of inside the method.
(also nit: let's assert `scalar64` as well, or none of the values at all
- I'd lean towards none since that part is already covered by the
regular decoding test).
> +
> + Ok(())
> + }
> +
> + /// Records each visit as (key, index, value), for tests on hand-built
> streams.
nit: `(key, index, value)`
> + #[derive(Default)]
> + struct Recorder<'d> {
> + visits: KVVec<(KeyId, u64, DecoderValue<'d>)>,
> + }
> +
> + impl<'d> Schema for Recorder<'d> {
> + type Target = KVVec<(KeyId, u64, DecoderValue<'d>)>;
> +
> + fn init() -> impl Init<Self> {
> + Self::default()
> + }
> +
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + Ok(core::mem::take(&mut self.visits))
> + }
> + }
> +
> + impl<'d> Visit<'d> for Recorder<'d> {
> + fn visit(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'d>) -> Result<bool> {
> + self.visits.push((key, index.get(), value), GFP_KERNEL)?;
> + Ok(true)
> + }
> + }
> +
> + // Tests the decoder on hand-built `u64` values that the encoder does
> not produce: SEQ32,
> + // multi-value SEQ64, zero counts, a non-zero index and padded arrays.
> + #[test]
> + fn decode_raw_u64s() -> Result {
> + const SEQ32_KEY: KeyId = 0x2000;
> + const SEQ64_KEY: KeyId = 0x2010;
> + const EMPTY_SEQ64_KEY: KeyId = 0x2020;
> + const EMPTY_SEQ32_KEY: KeyId = 0x2021;
> + const EMPTY_ARRAY8_KEY: KeyId = 0x2030;
> + const EMPTY_ARRAY32_KEY: KeyId = 0x2031;
> + const EMPTY_ARRAY64_KEY: KeyId = 0x2032;
> + const ARRAY8_KEY: KeyId = 0x2040;
> + const ARRAY32_KEY: KeyId = 0x2041;
> +
> + let index3 = Index::new::<3>();
nit: variable only used once, can be created inline.
I really like how testing coverage has improved in this revision!
