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

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

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 <ecourtney@nvidia.com>
> ---
>  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!