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

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

On Wed Oct 7, 2026 at 9:05 PM JST, Alexandre Courbot wrote:
> Sorry, some follow-up comments after my previous review.
>
> 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)]
>
> I see that `#[repr(transparent)]` is used several times in this series,
> but is there a need for it? Same for the many `#[inline]`s, here I feel
> like letting the compiler arrange things as it wants might be the better
> call. I know I suggested downgrading from always-inline to just inline,
> so maybe we should go all the way here. These are very likely to be
> inlined anyway, and worst case I don't think a function call would
> induce a big cost.

It's not required. I was adding that because I was thinking of these
types semantically as just being type-level only wrappers of the
underlying data. I'll remove them, plus the inlines.

>
> <...>
>> +impl<T: Default, const KEY_ID: KeyId> Schema for Key<T, KEY_ID> {
>> +    type Target = T;
>> +
>> +    #[inline]
>> +    fn init() -> impl Init<Self> {
>> +        Self::default()
>> +    }
>> +
>> +    #[inline]
>> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> +        Ok(core::mem::take(&mut self.0))
>> +    }
>> +}
>
> Both methods return the built value on the stack, which is fine when we
> only deal with scalars but technically we could also store larger
> values. How about a `const_assert!` ensuring we don't go beyond, say, 32
> bytes for the size of `T` and `Target`?

Will so do!

>
> <...>
>> +/// 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>);
>> +
>> +impl<T, const KEY_ID: KeyId> Schema for Required<T, KEY_ID> {
>> +    type Target = T;
>> +
>> +    #[inline]
>> +    fn init() -> impl Init<Self> {
>> +        Self(None.into())
>> +    }
>> +
>> +    #[inline]
>> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> +        (self.0).0.take().ok_or(EINVAL)
>
> I've experimented a bit with my earlier suggestion of having a wrapping
> `Required` type because I wasn't so sure it would work, but it seems to
> be indeed doable! Here is my draft implementation:
>
> pub(crate) struct Required<S: Schema> {
>     inner: S,
>     parsed: bool,
> }
>
> impl<S: Schema> Schema for Required<S> {
>     type Target = S::Target;
>
>     fn init() -> impl Init<Self> {
>         init!(Self { inner <- S::init(), parsed: false })
>     }
>
>     fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>         // Reset `self.parsed` to make the schema empty again per the method contract.
>         let parsed = core::mem::take(&mut self.parsed);
>         self.inner
>             .finish()
>             .chain(move |_| if parsed { Ok(()) } else { Err(EINVAL) })
>     }
> }
>
> impl<'data, S: Schema + Visit<'data>> Visit<'data> for Required<S> {
>     fn visit(&mut self, key: KeyId, index: Index, value: DecoderValue<'data>) -> Result<bool> {
>         let consumed = self.inner.visit(key, index, value)?;
>         self.parsed |= consumed;
>         Ok(consumed)
>     }
> }
>
> You need to convert all the `Required<T, ...>` into `Required<Key<T,
> ...>>`, but this reads more logically I think, and now you can combine
> `Required` with more types. I believe you could also implement
> `Optional` in a similar way.
>
> (you will also need to add a `#[derive(Default)]` to `FbRegionFlags` on
> the next patch, but that's not a big deal since it wraps a primitive
> type anyway)

Thanks for prototyping this, will add this in.