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 <[email protected]> >> --- >> 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.
