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

[Eliot Courtney]

On Wed Sep 9, 2026 at 1:48 PM JST, Alexandre Courbot wrote:
> 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.

Yes, I'll strengthen the wording, sgtm.