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

[Alexandre Courbot]

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:
>> 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.

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.