Re: [PATCH v3 7/8] gpu: nova-core: add NVKV typed decoding

["Eliot Courtney" <ecourtney@nvidia.com>]

On Wed Oct 7, 2026 at 3:30 PM JST, Alexandre Courbot wrote:
> On Mon Sep 28, 2026 at 5:42 PM JST, Eliot Courtney wrote:
>> Similar to the typed encoding layer, add some decoding type machinery.
>> Add a simple macro `nvkv_decode!` which implements `Schema` for a struct
>> by composing visit calls to each member. Add some common `Schema` kinds,
>> such as `Array` which collects an array value into a fixed maximum size
>> array, and `Required` which fails a decode if the value is not sent.
>>
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/gsp/nvkv.rs        |  11 +-
>>  drivers/gpu/nova-core/gsp/nvkv/decode.rs | 622 ++++++++++++++++++++++++++++++-
>>  2 files changed, 628 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
>> index 7ac3a459a98b..5791df07a7fa 100644
>> --- a/drivers/gpu/nova-core/gsp/nvkv.rs
>> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
>> @@ -9,7 +9,7 @@
>>  //! function calls will map to some struct - for example, f(GPU_NAME_STRING_KEY, 0, b"some gpu")
>>  //! naturally maps to storing a &str with the GPU name.
>>  
>> -#![expect(unused_imports)]
>> +#![cfg_attr(not(CONFIG_KUNIT), expect(unused_imports))]
>>  #![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>>  
>>  use core::{
>> @@ -23,7 +23,8 @@
>>  use kernel::{
>>      alloc::{
>>          allocator::KVmalloc,
>> -        Allocator, //
>> +        Allocator,
>> +        ArrayVec, //
>>      },
>>      bitfield,
>>      num::Bounded,
>> @@ -148,6 +149,12 @@ fn default() -> Self {
>>      }
>>  }
>>  
>> +/// A schema field for an array value under the NVKV key `KEY_ID`.
>> +#[repr(transparent)]
>> +pub(crate) struct Array<T: Default + Copy, const N: usize, const KEY_ID: KeyId> {
>> +    vec: ArrayVec<T, N>,
>> +}
>
> I still don't see why this needs to be declared in this module when no
> encoder element ever uses it, and the module reexports everything from
> `decode` anyway. Can this be moved next to the other schema kinds?

This will be used soon [1] for encoding. The convention I have been
using is if a type is used for both encoding and decoding, I put it in
nvkv.rs (since it doesn't fit obviously in either - the
encoding/decoding related impls can go in there though). If we put
IndexedKey in nvkv.rs as pub crate since we'll use it soon, it's
consistent to keep Array here as well. WDYT?

[1]: https://lore.kernel.org/all/473ff4cbabf401357934a611c0310499130d9efe.1790580105.git.zhiw@nvidia.com/

>
>> +
>>  bitfield! {
>>      /// The op word that starts each NVKV operation.
>>      struct Op(u64) {
>> diff --git a/drivers/gpu/nova-core/gsp/nvkv/decode.rs b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
>> index c4c24fe1108e..24dad31296cb 100644
>> --- a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
>> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
>> @@ -3,11 +3,22 @@
>>  
>>  #![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>>  
>> -use kernel::prelude::*;
>> +use core::{
>> +    convert::Infallible,
>> +    marker::PhantomData, //
>> +};
>> +
>> +use kernel::{
>> +    alloc::ArrayVec,
>> +    prelude::*, //
>> +};
>> +use pin_init::init_array_from_fn;
>>  
>>  use crate::{
>>      gsp::nvkv::{
>> +        Array,
>>          Index,
>> +        Key,
>>          KeyId,
>>          Op,
>>          Opcode, //
>> @@ -15,6 +26,353 @@
>>      num, //
>>  };
>>  
>> +/// Defines a schema struct together with its [`Schema`] and [`Visit`] implementations that decode
>> +/// into `$target`.
>> +///
>> +/// Each member of the struct should implement [`Schema`] and [`Visit`]. For every (key, index,
>> +/// value) triple decoded from the NVKV stream, the generated parent `Visit` implementation will
>
> nit: let's put backticks around (key, index, value) and link to `Visit`
> (and other types mentioned in this doccomment, and possibly elsewhere in
> this patch).

Done (here and other locations).

>
> <...>
>> +/// A schema field for a key that must be present.
>> +///
>> +/// `finish` fails with `EINVAL` if no value arrived for the key.
>> +#[repr(transparent)]
>> +pub(crate) struct Required<T, const KEY_ID: KeyId>(Key<Option<T>, KEY_ID>);
>
> I just noticed that the schema field types are conflating several
> concepts together. On the one hand, you have types associating a key to
> some kind of storage (Key, Array, Indexed - let's call these leaf
> types), and on the other what looks like modifiers on said leaf types
> (Accumulated).
>
> Which leaves `Required` somewhere in between, as it takes a key
> parameter and can only be applied to value types, i.e. you currently
> cannot have a `Required<Array<...>>`. But `Accumulated<Array<...>>` does
> work IIUC.
>
> So I think `Required` should work the same way as `Accumulated`, i.e.
> just wrap it around the type you want to make required, instead of
> switching the leaf type.
>
> And for symmetry, we should also have an `Optional` wrapper type, so we
> can also make fields optional even if they are not `Key`s instead of
> having specific behavior for `Key<Option>` (which again is not
> composable).
>
> Which would give us 3 leaf types (Key, Array, Indexed) and 3 modifiers
> (Required, Optional, Accumulated), with a non-wrapped leaf meaning
> "default if unspecified" - which is easier to understand than the
> current types where `Required` is a synonym for "a `Key` except it's
> required".
>
> This means `Required` would then be used like `Required<Key<...>>`,
> which is a bit more verbose, but again nicer to read.

Yerp I think that is fine. I originally did it this way to avoid the
verbosity you mention ("Required<Key...>") and to avoid needing Default
implementations, but I'm fine with that version too. Let me update it.

>
> <...>
>>  /// A decoded NVKV value.
>>  #[derive(Copy, Clone, Debug, PartialEq, Eq)]
>>  pub(crate) enum DecoderValue<'a> {
>> @@ -51,7 +409,16 @@ fn try_from(value: DecoderValue<'a>) -> Result<Self> {
>>  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`.
>
> Why change this doccomment? Let's introduce its final form in patch 5.

Thanks for spotting, think I did not absorb the change into the correct
commit.

>
>> +/// Lets `Key<Option<T>, KEY_ID>` accept whatever `Key<T, KEY_ID>` accepts.
>> +impl<'a, T: TryFrom<DecoderValue<'a>, Error = Error>> TryFrom<DecoderValue<'a>> for Option<T> {
>> +    type Error = Error;
>> +
>> +    fn try_from(value: DecoderValue<'a>) -> Result<Self> {
>> +        T::try_from(value).map(Some)
>> +    }
>> +}
>
> IIUC this block could go away if we add an `Optional` wrapper suggested
> above.

Think this is ok, one issue with `Optional` is that it's currently not
possible to pin_init (or init) an Option without materializing the value
that goes in there on the stack. It's an issue either way though but the
`TryFrom<DecoderValue>` limits it to small values (refs or small integer
types). Anyway, I think we can just add another assert on the size for
now, so SGTM.

>
>> +
>> +/// The state of one NVKV decode operation which produces a target value `Target`.
>>  pub(crate) trait Schema {
>>      type Target;
>>  
>> @@ -65,7 +432,12 @@ fn init() -> impl Init<Self>
>>  
>>      /// Returns an initializer that makes the decoded `Target`.
>>      ///
>> -    /// After the returned initializer runs, the schema should be empty again.
>> +    /// After the returned initializer runs successfully, the schema must be empty again. For
>> +    /// example, this is required by [`Accumulated`] which finishes one object and then decodes the
>> +    /// next one with the same schema. Implementations generated by `nvkv_decode!` meet this
>> +    /// requirement. If the initializer fails, the `Schema` can be in a valid but non-fresh state.
>> +    /// Taking `self` instead of `&mut self` would avoid this contract, but it forces a copy of the
>> +    /// schema onto the stack.
>
> Let's not cite specific implementations in the doccomment of a trait. We
> want to say that the schema *must* be empty (as opposed to "should")
> from patch 5, and that should be clear enough and sufficient. I'd also
> move the last sentence to patch 5, and drop the rest.

Done

[...]
>> +        assert_eq!(decoded.slots, [10, 20, 30, 40]);
>> +
>> +        Ok(())
>> +    }
>> +
>> +    // Tests that a schema too large for the stack decodes on the heap.
>
> How are we testing this? Do we expect the stack to explode as a test
> failure? That should be guaranteed by the use of `KBox` and `init`, so I
> am not sure there is much to test here...
>
> But if you decide to keep it, let's at least size the schema so the
> stack *actually* explodes: 2KB is not enough, we want at least 16KB, and
> for good measure let's go with at least 128KB to be sure. Otherwise I
> think it's fine to drop this test.

Happy to drop this test.