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

["Alexandre Courbot" <acourbot@nvidia.com>]

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.

<...>
> +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`?

<...>
> +/// 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)