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

[Eliot Courtney]

On Wed Sep 9, 2026 at 9:51 AM JST, Alexandre Courbot wrote:
> On Thu Aug 27, 2026 at 11:12 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` 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 <ecourtney@xxxxxxxxxx>
>> ---
>>  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.