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